chore: update spec.types.ts from upstream#2027
Conversation
|
fe613a8 to
5f450ff
Compare
a64a23e to
af35bb5
Compare
| /* Request parameter type that includes input responses and request state. | ||
| * These parameters may be included in any client-initiated request. | ||
| */ | ||
| export interface InputResponseRequestParams extends RequestParams { | ||
| /* New field to carry the responses for the server's requests from the | ||
| * InputRequiredResult message. For each key in the response's inputRequests | ||
| * field, the same key must appear here with the associated response. | ||
| */ | ||
| inputResponses?: InputResponses; | ||
| /* Request state passed back to the server from the client. | ||
| */ | ||
| requestState?: string; | ||
| } |
There was a problem hiding this comment.
🟡 Nit (upstream): the field/interface comments here use plain /* ... */ instead of /** ... */ JSDoc, so the TS language service / TypeDoc will not surface them — and InputResponseRequestParams ends up with no doc comment and no @category Multi Round-Trip tag at all, unlike every sibling type in this section. Since spec.types.ts is not re-exported from this package the SDK's own docs are unaffected, but it's worth folding into the same upstream schema.ts fix as the resultType issue so the spec repo's generated docs and the forthcoming hand-written schemas.ts mirror get proper descriptions.
Extended reasoning...
What the issue is
In the new Multi Round-Trip block, several comments use plain block-comment syntax /* ... */ instead of JSDoc syntax /** ... */:
InputRequiredResult.inputRequests(lines 394-396)InputRequiredResult.requestState(lines 398-402)- the interface-level comment on
InputResponseRequestParams(lines 406-408) InputResponseRequestParams.inputResponses(lines 410-413)InputResponseRequestParams.requestState(lines 415-416)
Because the opening delimiter is /* (single asterisk) rather than /**, TypeScript's language service and TypeDoc treat these as ordinary comments, not documentation. Additionally, since InputResponseRequestParams has no /** */ block at all, it also has no @category Multi Round-Trip tag — every other exported type in this section (InputRequests, InputResponses, InputRequiredResult, TaskInputResponseRequest, etc.) carries that tag.
Why this is an upstream slip, not intentional
This is not the file's convention. Every surrounding declaration in the same diff hunk uses proper /** ... */ JSDoc: ResultType (148-156), InputRequests (354-362), InputResponses (366-375), InputRequiredResult itself (380-392), TaskInputResponseRequest (2031-2039), TaskInputResponseRequestParams (2046-2056). The pre-existing /* Empty result */ and /* Cancellation */ lines are one-line section dividers, not API documentation, so they are not precedent for multi-line field descriptions. The five blocks above are the only multi-line API descriptions in the file using /* — a clear authoring inconsistency in the upstream commit.
Addressing the "not SDK-public" objection
It is true that spec.types.ts is not part of this SDK's public surface — packages/core/src/types/index.ts re-exports constants/enums/errors/guards/schemas/specTypeSchema/types but not spec.types, and the only importer in the package is test/spec.types.test.ts. So this has zero effect on the typescript-sdk's generated docs or consumer .d.ts, and CLAUDE.md's "JSDoc for public APIs" rule does not directly apply to this file in this repo.
The reason it is still worth a (nit-level) mention is that this file is a verbatim mirror of the spec repo's schema/draft/schema.ts, which is the source for the protocol's own TypeDoc site. In the upstream output, InputResponseRequestParams will render with no description and will be uncategorised (it will not appear under the "Multi Round-Trip" group), and the normative "client must treat this as an opaque blob" guidance on requestState will be invisible in IDE hover for anyone consuming the spec types. The fix lives in the same upstream file that already needs editing for the resultType? optionality issue flagged elsewhere on this PR, so the marginal cost of including it in that upstream report is near zero.
Step-by-step proof
- Hover
InputResponseRequestParamsat line 409 in VS Code → tooltip shows onlyinterface InputResponseRequestParams extends RequestParamswith no description, because lines 406-408 start with/*not/**. - Hover
InputRequiredResultat line 393 → tooltip shows the full "An InputRequiredResult sent by the server…" text, because lines 380-392 start with/**. - Hover
requestStateat line 403 → no description; the "client must treat this as an opaque blob" note (398-402) is dropped. - Run TypeDoc over upstream
schema.ts→InputRequests,InputResponses,InputRequiredResultare grouped under Multi Round-Trip;InputResponseRequestParamsis not (no@categorytag, because there is no JSDoc block to carry one).
How to fix
In upstream schema.ts, change /* → /** on the five blocks listed above and add @category Multi Round-Trip to the InputResponseRequestParams doc comment, then re-run pnpm run fetch:spec-types. No change is appropriate in this repo directly (the file header says DO NOT EDIT). This is purely documentation rendering — it does not affect type-checking, the drift guard, or runtime behaviour — hence nit, raised only because an upstream schema.ts fix is already on the table for this sync.
| export interface InputRequiredResult extends Result { | ||
| /* Requests issued by the server that must be complete before the | ||
| * client can retry the original request. | ||
| */ | ||
| inputRequests?: InputRequests; | ||
| /* Request state to be passed back to the server when the client | ||
| * retries the original request. | ||
| * Note: The client must treat this as an opaque blob; it must not | ||
| * interpret it in any way. | ||
| */ | ||
| requestState?: string; | ||
| } |
There was a problem hiding this comment.
🟡 Upstream design gap worth flagging alongside the other schema.ts feedback: InputRequiredResult extends Result without redeclaring resultType: 'input_required', and none of the "complete" subtypes (CallToolResult/ReadResourceResult/GetPromptResult/GetTaskPayloadResult) narrow to resultType: 'complete' either — so the new result: CallToolResult | InputRequiredResult unions (lines 1122/1455/1650/2028) are not TypeScript discriminated unions and if (r.resultType === 'input_required') will not narrow. This is orthogonal to the "resultType is required" comment above (fixing one doesn't fix the other) and constrains the SDK from modeling these with z.discriminatedUnion('resultType', ...) while keeping the bidirectional spec↔SDK assignability check green.
Extended reasoning...
What the bug is
The spec introduces ResultType = 'complete' | 'input_required' and adds resultType: ResultType to the base Result interface (line 165), with the JSDoc explicitly stating its purpose is to "allow the client to determine how to parse the result object." It then defines InputRequiredResult extends Result (line 393) and unions it into four response envelopes — CallToolResultResponse.result: CallToolResult | InputRequiredResult (1650), and likewise for ReadResourceResultResponse (1122), GetPromptResultResponse (1455), and GetTaskPayloadResultResponse (2028).
However, InputRequiredResult does not redeclare resultType: 'input_required', and none of CallToolResult / ReadResourceResult / GetPromptResult / GetTaskPayloadResult redeclare resultType: 'complete'. A grep confirms resultType appears exactly once in spec.types.ts — only on the base Result. So every arm of every X | InputRequiredResult union has resultType typed as the full 'complete' | 'input_required', and TypeScript's discriminated-union narrowing does not engage.
Step-by-step proof
Result.resultType: 'complete' | 'input_required'(line 165).InputRequiredResult extends Result { inputRequests?: ...; requestState?: ... }(line 393) — inheritsresultType: 'complete' | 'input_required'unchanged.CallToolResult extends Result { content: ...; ... }— also inheritsresultType: 'complete' | 'input_required'unchanged.- Given
declare const r: CallToolResult | InputRequiredResult:TypeScript cannot eliminateif (r.resultType === 'input_required') { r.inputRequests; // ❌ TS error: Property 'inputRequests' does not exist on type 'CallToolResult | InputRequiredResult' }
CallToolResultfrom the union becauseCallToolResult['resultType']also includes'input_required'. Narrowing requires the discriminant property to have disjoint literal types across union members. - Additionally, since
Resultcarries[key: string]: unknownandInputRequiredResult's only additions (inputRequests?,requestState?) are optional,InputRequiredResultis structurally a subtype ofCallToolResult— the union is effectively degenerate at the type level.
Why this is distinct from the existing "resultType is required" comment
The comment on line 165 is about resultType being declared required despite @default "complete", which breaks SDK→spec assignability for every result. This finding is about resultType not being narrowed on subtypes, which breaks discriminated-union narrowing within the spec types themselves. They are orthogonal: making resultType optional on Result does not give InputRequiredResult a narrowed discriminant, and adding resultType: 'input_required' to InputRequiredResult does not make the base field optional. Both should be raised upstream together.
Impact on the SDK
The companion schemas.ts work (already requested in another comment on this PR) will need to define InputRequiredResultSchema and the four result: X | InputRequiredResult unions. The natural Zod encoding is z.discriminatedUnion('resultType', [...]), which requires each arm to declare a z.literal(...) discriminant. But if the SDK narrows InputRequiredResultSchema to resultType: z.literal('input_required'), the spec→SDK direction of the bidirectional assignability check in spec.types.test.ts fails: the spec's InputRequiredResult['resultType'] is 'complete' | 'input_required', which is not assignable to the SDK's 'input_required'. So the SDK is forced to either (a) use a non-discriminated z.union and lose narrowing, or (b) narrow anyway and add a carve-out in the bidirectional test — neither is great, and both go away if upstream narrows the discriminant.
At runtime the wire-level discriminator still works (a client can string-compare resultType and cast), so this is a type-ergonomics / SDK-modeling defect rather than a protocol-correctness one.
How to fix
Upstream in schema.ts: add resultType: 'input_required'; to InputRequiredResult, and add resultType?: 'complete'; (or required 'complete', depending on how the optionality question is resolved) to each concrete "complete" result that participates in an | InputRequiredResult union — at minimum CallToolResult, ReadResourceResult, GetPromptResult, GetTaskPayloadResult. Then re-run pnpm run fetch:spec-types. Batch this with the resultType? optionality fix and the other upstream feedback already noted on this PR.
af35bb5 to
879139a
Compare
| /* Empty result */ | ||
| /** | ||
| * A result that indicates success but carries no data. | ||
| * | ||
| * @category Common Types | ||
| */ | ||
| export type EmptyResult = Result; | ||
|
|
||
| /** @internal */ | ||
| export type InputRequest = CreateMessageRequest | ListRootsRequest | ElicitRequest; | ||
|
|
There was a problem hiding this comment.
🔴 🔴 The spec deletes URL_ELICITATION_REQUIRED = -32042 / URLElicitationRequiredError (replaced by the InputRequiredResult flow), but the SDK still ships this mechanism as public API: ProtocolErrorCode.UrlElicitationRequired (enums.ts:15), the UrlElicitationRequiredError class + ProtocolError.fromError() special-case (errors.ts:23-48), the public re-export (exports/public/index.ts:103), the tool-handler rethrow at packages/server/src/server/mcp.ts:211, and the examples/{client,server}/src/elicitationUrlExample.ts apps (plus docs/README links and test/integration/test/server/mcp.test.ts:1892-1937). None of these import spec.types.ts, so unlike the other findings on this PR they are silent — no typecheck or drift-guard failure surfaces them. The companion work needs to deprecate/remove the public error class + enum member (breaking change → migration.md), strip the mcp.ts special-case, and rewrite/remove the elicitationUrlExample apps.
Extended reasoning...
What changed in the spec
This sync deletes the entire -32042 error-response mechanism from the protocol. Before, the spec defined an implementation-specific JSON-RPC error code URL_ELICITATION_REQUIRED = -32042 and a URLElicitationRequiredError interface (a JSONRPCErrorResponse whose error.data.elicitations carried ElicitRequestURLParams[]). A server could respond to tools/call with this error to demand that the client open a browser URL before retrying. The diff removes both the constant and the interface; the replacement is the new InputRequiredResult result (with resultType: 'input_required' and inputRequests), which is delivered as a successful response, not an error. The error path is gone from the protocol entirely.
What the SDK still ships
The SDK implemented the -32042 flow end-to-end and exported it publicly. All of the following survive untouched after this PR:
packages/core/src/types/enums.ts:15—ProtocolErrorCode.UrlElicitationRequired = -32_042, an error code the spec no longer defines.packages/core/src/types/errors.ts:21-48—ProtocolError.fromError()special-cases code-32042and constructs aUrlElicitationRequiredError; theUrlElicitationRequiredErrorclass itself wrapselicitations: ElicitRequestURLParams[]intoerror.data.packages/core/src/exports/public/index.ts:103—export { ProtocolError, UrlElicitationRequiredError }puts the class on the package's public surface.packages/server/src/server/mcp.ts:211-213— thetools/callhandler catches thrown errors and, iferror.code === ProtocolErrorCode.UrlElicitationRequired, rethrows so the framework serialises it onto the wire as a JSON-RPC error response instead of wrapping it into aCallToolResultwithisError: true. This is runtime behaviour that emits a non-spec error code.examples/server/src/elicitationUrlExample.ts:58,102— tool handlersthrow new UrlElicitationRequiredError([...]).examples/client/src/elicitationUrlExample.ts:26,741— client catchesUrlElicitationRequiredErrorand drives the browser flow.test/integration/test/server/mcp.test.ts:1892-1937— integration test asserting the round-trip.docs/client.md:471,626,docs/server.md:477,examples/{client,server}/README.md— documentation linking to the example apps.
Why this is silent (and distinct from the other comments)
Every other finding on this PR is surfaced by spec.types.test.ts or tsc because the affected code references spec.types.ts. This one is not: enums.ts, errors.ts, mcp.ts, and the example apps do not import spec.types.ts. The enum member is a hand-written numeric literal (-32_042); the error class is a hand-written subclass of ProtocolError; the mcp.ts rethrow checks the enum, not the spec interface. Nothing in CI fails when URL_ELICITATION_REQUIRED and URLElicitationRequiredError are removed from spec.types.ts. A reviewer addressing only the existing comments would fix the schemas, the unions, and the test allowlists — and ship a release that still publicly exports an error class for a protocol mechanism that no longer exists.
This is also not a duplicate of the existing comments. Comment #3206453743 (line 3291) covers the server→client request restructuring (createMessage/elicitInput/listRoots becoming InputRequiredResult payloads) and its remediation list is scoped to ServerRequestSchema/ClientResultSchema and the server.createMessage()/elicitInput()/listRoots() APIs — it never mentions the -32042 error-response path, which is a completely separate code path (a tool handler throws, mcp.ts rethrows onto the wire as a JSON-RPC error). Comment #3206453749 (line 418) mentions URLElicitationRequiredError only as a stale entry in the MISSING_SDK_TYPES allowlist affecting the spec.types.test.ts type count; it does not mention enums.ts, errors.ts, the public re-export, mcp.ts:211, or the example apps.
Step-by-step proof
- Before (spec.types.ts pre-diff):
export const URL_ELICITATION_REQUIRED = -32042;andexport interface URLElicitationRequiredError extends Omit<JSONRPCErrorResponse, 'error'> { error: Error & { code: typeof URL_ELICITATION_REQUIRED; data: { elicitations: ElicitRequestURLParams[]; ... } } }. - After (this diff): both are deleted; the only remaining server-side mechanism for "need browser-based input before continuing" is to return
{ resultType: 'input_required', inputRequests: { ... } }as a successful result. - SDK runtime (
mcp.ts:200-215): a tool handler runs, throwsnew UrlElicitationRequiredError([...]). The catch block at line 211 seeserror.code === ProtocolErrorCode.UrlElicitationRequired(i.e.,-32_042) and rethrows.Protocolthen serialises this into a JSON-RPC error response witherror.code = -32042anderror.data.elicitations = [...]. - Wire: the SDK is now emitting an error code and error-data shape that the protocol no longer defines. A spec-compliant client built against the post-sync schema has no
URLElicitationRequiredErrortype to deserialise this into and no-32042constant to switch on. - Public surface:
import { UrlElicitationRequiredError } from '@modelcontextprotocol/sdk'still works (exports/public/index.ts:103), andexamples/server/src/elicitationUrlExample.tsactively demonstrates throwing it from a tool handler — teaching consumers a pattern the protocol just removed. - No CI signal:
grep -r 'spec.types' packages/core/src/types/{enums,errors}.ts packages/server/src/server/mcp.tsreturns nothing — these files have no compile-time link to the spec snapshot, so neithertscnorspec.types.test.tsflags them.
Impact
After this sync the SDK publicly exports, documents, demonstrates in two example apps, and special-cases at runtime a protocol mechanism that the spec has removed. New consumers following docs/server.md and elicitationUrlExample.ts will build servers that emit non-spec error responses; spec-compliant clients will see an unrecognised -32042 error instead of an InputRequiredResult they know how to fulfil.
How to fix
This is non-mechanical, public-API companion work that must accompany the sync (or its companion PR):
- Decide deprecate-vs-remove for
UrlElicitationRequiredErrorandProtocolErrorCode.UrlElicitationRequired. Either remove them outright (breaking → add amigration.mdentry and a major changeset) or mark them@deprecatedwith JSDoc pointing at theInputRequiredResultreplacement and schedule removal. - Strip the
mcp.ts:211-213rethrow special-case (and theProtocolError.fromError()branch aterrors.ts:23-28). - Rewrite or remove
examples/{client,server}/src/elicitationUrlExample.tsto use theInputRequiredResult/inputResponsesflow (this dovetails with the redesign already requested in comment #3206453743), and update the four README/docs links that point at them. - Delete
test/integration/test/server/mcp.test.ts:1892-1937(or rewrite it for the new flow).
There was a problem hiding this comment.
Status update — the framing of this comment has been partially superseded on main, but the SDK-side surface it lists is still untouched.
What changed since this was filed:
- The single
spec.types.tsthis comment anchored on no longer exists — feat(core): per-revision spec reference types and the 2026-07-28 wire contract surface #2252 split it into per-revision snapshots.spec.types.2025-11-25.tsstill definesURL_ELICITATION_REQUIRED = -32042/URLElicitationRequiredError, and the SDK's wire surface still targets that revision (LATEST_PROTOCOL_VERSION = '2025-11-25'in constants.ts). The latest re-pull in this PR also adds explicit prose to the 2026-07-28 snapshot reserving the code: "-32042(URL elicitation required, 2025-11-25 only)". So the original "the SDK emits an error code the protocol no longer defines" framing no longer holds for an SDK speaking 2025-11-25 — the mechanism is anchored to a still-supported, still-snapshotted protocol revision, consistent with the deliberate retain-the-2025-11-25-wire-surface decision (fix(types): restore task wire types removed with the task feature #2248).
What remains untouched (the companion-work residue, narrowed to deprecate-and-migrate rather than remove):
ProtocolErrorCode.UrlElicitationRequired = -32_042(enums.ts:25), theUrlElicitationRequiredErrorclass +ProtocolError.fromError()special-case (errors.ts:23-56), and the public re-export (exports/public/index.ts:97) all survive with no@deprecatedJSDoc — unlike the SEP-2577 sampling/roots/logging tier, which did get deprecation annotations.- The
tools/callrethrow special-case is still live (now packages/server/src/server/mcp.ts:174). examples/{client,server}/src/elicitationUrlExample.ts, the docs links (docs/server.md:514, docs/client.md:479/677, both example READMEs), and the integration test (test/integration/test/server/mcp.test.ts:1869-1912) still demonstrate/assert the -32042 flow with no note that it is a 2025-11-25-only mechanism replaced byInputRequiredResultin 2026-07-28.
When the 2026-07-28 companion implementation lands, this surface needs version-aware handling: @deprecated/JSDoc pointing at the InputRequiredResult replacement, a decision on whether the mcp.ts rethrow is gated by negotiated protocol revision, and migration of the elicitationUrlExample apps + docs to the new flow — but outright removal is no longer the right ask given the 2025-11-25 interop window.
| export type InputRequest = CreateMessageRequest | ListRootsRequest | ElicitRequest; | ||
|
|
||
| /** @internal */ | ||
| export const URL_ELICITATION_REQUIRED = -32042; | ||
| export type InputResponse = CreateMessageResult | ListRootsResult | ElicitResult; |
There was a problem hiding this comment.
🟡 Upstream design gap to batch with the other schema.ts feedback: InputResponse = CreateMessageResult | ListRootsResult | ElicitResult admits only success payloads, and the InputResponseRequestParams.inputResponses comment says every inputRequests key "must appear here". In the pre-sync model these were full JSON-RPC requests so a client could return a JSONRPCErrorResponse for user-denied sampling / LLM provider error / no roots configured; that error channel is gone with no replacement (ElicitResult has action: 'decline'|'cancel', but CreateMessageResult and ListRootsResult have no refusal field). The SDK redesign called for in the earlier comment will have no spec-defined way to propagate per-request failures and would have to invent an out-of-spec workaround that breaks once upstream adds a real error variant — suggest upstream add an InputResponseError arm (mirroring the JSON-RPC {code, message, data} shape) or relax the must-appear rule.
Extended reasoning...
What the bug is
InputResponse (spec.types.ts:351) is defined as CreateMessageResult | ListRootsResult | ElicitResult — a union of three success payloads only. InputResponses (line 376-378) is { [key: string]: InputResponse }, and the normative-sounding comment on InputResponseRequestParams.inputResponses (lines 410-413) says: "For each key in the response's inputRequests field, the same key must appear here with the associated response." So per the spec text, the client must supply a value for every requested key, and that value must be one of the three success result shapes.
In the pre-sync model these three exchanges were ordinary server→client JSON-RPC requests (CreateMessageRequest extends JSONRPCRequest, etc.), so a client that could not or would not fulfil one returned a JSONRPCErrorResponse carrying { code: number; message: string; data?: unknown }. By stripping extends JSONRPCRequest / extends Result and embedding the exchange inside the InputRequests/InputResponses maps, the spec discarded that error channel without adding a replacement.
Why the existing types don't cover it
ElicitResulthappens to carryaction: 'accept' | 'decline' | 'cancel', so elicitation can encode refusal in-band.CreateMessageResult(line ~2335) isSamplingMessage & { model: string; stopReason?: ... }— requiredmodel/role/content, no refusal/error field, no index signature now thatextends Resultis dropped.ListRootsResult(line ~2826) is bare{ roots: Root[] }— same story.
The asymmetry (only ElicitResult has a decline arm) suggests this is an oversight rather than an intentional "refusal-via-abandonment" design — if abandoning the retry were the intended refusal signal, ElicitResult would not need action: 'decline' either.
Step-by-step proof
- Server returns
CallToolResultResponsewithresult: InputRequiredResult { inputRequests: { 's1': <CreateMessageRequest>, 'e1': <ElicitRequest> } }. - Client presents the elicitation; user accepts →
{ action: 'accept', content: {...} }. - Client attempts the sampling call; the LLM provider returns HTTP 429 / content-policy refusal / the user denies the sampling-consent prompt.
- Client must now construct
inputResponsesfor the retry. Per lines 410-413, both's1'and'e1'must appear.'e1'is fine. For's1'the only spec-permitted shapes areCreateMessageResult | ListRootsResult | ElicitResult— none of which can express "this request failed with code X / message Y". - The client's only options are: (a) omit
's1'— textually non-compliant with the must-appear rule; (b) abandon the whole retry / calltasks/cancel— loses the successful elicitation and gives the server no error code/message to act on; (c) stuff a fakeCreateMessageResultor an off-spec{ error: {...} }object into's1'— out of spec.
Why this matters for the SDK companion work
Today a client-side setRequestHandler(CreateMessageRequestSchema, …) can throw / reject and the SDK serialises a JSONRPCErrorResponse; the server's await ctx.sample() rejects with a ProtocolError carrying the code and message, and the tool handler can catch it and fall back. The redesign called for in comment #1 must preserve some equivalent of those semantics, but there is no spec-defined wire shape to carry them. Whatever workaround the SDK picks (drop the key, cancel the whole flow, invent an envelope) becomes public API that breaks once upstream adds a real error variant.
This is distinct from comment #1 (about the SDK still issuing these as wire requests), comment #4 (about tasks/input_response lacking taskId), and comment #6 (about the resultType discriminant) — none of those address the missing error arm in the InputResponse payload union itself.
Addressing "all-or-nothing is the intended design"
It is plausible the spec intends atomic fulfilment (client either satisfies every input or abandons). But (a) that loses the error code/message — the server cannot distinguish rate-limit vs. content-policy vs. user-denied, which is a real expressiveness regression vs. the JSON-RPC model; (b) it is contradicted by ElicitResult.action: 'decline'|'cancel' existing at all; and (c) even under that reading, the SDK still has to pick a public error-propagation contract for ctx.sample() rejection in the new flow, and it is better to do so against a spec-defined error variant than to guess.
How to fix
Raise upstream alongside the resultType? / taskId / discriminant fixes: either add an error arm, e.g.
export interface InputResponseError {
code: number;
message: string;
data?: unknown;
}
export type InputResponse = CreateMessageResult | ListRootsResult | ElicitResult | InputResponseError;(mirroring the removed JSON-RPC Error shape), or relax the "must appear" wording on inputResponses and define omission as refusal. Then re-run pnpm run fetch:spec-types. Don't encode InputResponsesSchema in schemas.ts until this is settled. Filed as a nit because it doesn't independently break CI and abandon-the-flow is a viable (if lossy) fallback — same tier as comments #5/#6.
879139a to
81ef4d8
Compare
81ef4d8 to
29aa626
Compare
| export interface SubscriptionsListenRequest extends JSONRPCRequest { | ||
| method: 'subscriptions/listen'; | ||
| params: SubscriptionsListenRequestParams; | ||
| } |
There was a problem hiding this comment.
🟡 🟡 Upstream spec gap to batch with the other schema.ts feedback: SubscriptionsListenRequest extends JSONRPCRequest (so it carries an id and per JSON-RPC 2.0 must eventually receive exactly one response object), but unlike every other *Request extends JSONRPCRequest in this file it has no SubscriptionsListenResult / SubscriptionsListenResultResponse, and ServerResult includes none. The spec instead has the server send SubscriptionsAcknowledgedNotification "as the first message on the stream" — a notification (no id), not a response — so the wire contract for completing/closing the subscriptions/listen request is undefined, and when the SDK adds SubscriptionsListenRequestSchema it will have no spec-defined result schema to pair with Protocol.request() (the pending-request map would leak the id if the server never responds). Suggest upstream either add SubscriptionsListenResultResponse { result: EmptyResult } (sent on stream close) or document that this request is intentionally never JSON-RPC-responded so the SDK can special-case its id bookkeeping.
Extended reasoning...
What the bug is
SubscriptionsListenRequest (spec.types.ts:1223) is declared as:
export interface SubscriptionsListenRequest extends JSONRPCRequest {
method: 'subscriptions/listen';
params: SubscriptionsListenRequestParams;
}Because it extends JSONRPCRequest, it carries jsonrpc: '2.0' and id: RequestId. Per JSON-RPC 2.0 §4.2, a request object that includes an id MUST eventually receive exactly one response object (either a result or an error) carrying that same id. But the spec defines no SubscriptionsListenResult or SubscriptionsListenResultResponse, and ServerResult (line 3296+) has no member for it. The only server reaction the spec defines is SubscriptionsAcknowledgedNotification — "sent by the server as the first message on a subscriptions/listen stream" — which extends JSONRPCNotification and therefore has no id and cannot satisfy the JSON-RPC response requirement.
Why this is uniquely inconsistent
Grepping the file shows 11 concrete interfaces that extends JSONRPCRequest (DiscoverRequest, PaginatedRequest and its derivatives, ReadResourceRequest, SubscriptionsListenRequest, GetPromptRequest, CallToolRequest, GetTaskRequest, GetTaskPayloadRequest, TaskInputResponseRequest, CancelTaskRequest, CompleteRequest) and 15 *ResultResponse extends JSONRPCResultResponse wrappers. Every JSONRPCRequest subtype has a paired *ResultResponse — except SubscriptionsListenRequest. The deleted predecessors it replaces (SubscribeRequest / UnsubscribeRequest) both had explicit { result: EmptyResult } wrappers (SubscribeResultResponse / UnsubscribeResultResponse), so this is a regression in spec completeness, not an established convention for "streaming" requests. ServerResult does include EmptyResult, so a server could respond with that on stream close, but the spec does not say so and there is no SubscriptionsListenResultResponse documenting it.
Step-by-step proof
- Client sends
{ jsonrpc: '2.0', id: 7, method: 'subscriptions/listen', params: { notifications: {...} } }(this is a JSON-RPC request becauseSubscriptionsListenRequest extends JSONRPCRequest, line 1223). - Server replies with
{ jsonrpc: '2.0', method: 'notifications/subscriptions/acknowledged', params: { notifications: {...} } }(line 1255 —extends JSONRPCNotification, noid). - Per JSON-RPC 2.0, message (2) is a notification, not a response to
id: 7. The client's pending-request entry forid: 7is still outstanding. - The spec defines no message that ever carries
id: 7back. There is noSubscriptionsListenResultResponse;ServerResulthas no dedicated arm; the JSDoc onSubscriptionsListenRequestdoes not say "the server never responds" or "the server responds withEmptyResulton stream close". - Three behaviours are therefore equally spec-compliant and mutually incompatible: (a) server sends
{ id: 7, result: {} }immediately and then streams notifications; (b) server sends{ id: 7, result: {} }only when the stream closes; (c) server never responds and the client must special-case theid. The wire contract is undefined.
Concrete SDK impact
The SDK's Protocol.request() API takes a request and a result schema, stores the id in a pending-request map, and resolves the returned promise when a matching response arrives. When the companion work (already requested in comment #3223937253) adds SubscriptionsListenRequestSchema, there is no spec-defined result schema to pass as the second argument. If the SDK uses EmptyResultSchema and a server follows interpretation (c), the promise never resolves and the pending-request map leaks id: 7 for the lifetime of the connection. If the SDK special-cases this method to not await a response and a server follows interpretation (a), the SDK will receive an unsolicited { id: 7, result: {} } it has no handler for. Either way, encoding SubscriptionsListenRequestSchema before this is settled bakes a guess into the public surface that breaks once upstream picks one.
Why this is distinct from existing PR comments
Comment #3223937253 lists SubscriptionsListenRequest/SubscriptionsListenRequestParams among the "new types needing SDK schemas" but does not note that there is no response type to pair them with — it assumes mechanical schema work, which is precisely what this gap blocks. Comment #3223937258 covers the SDK lifecycle redesign (subscribeResource()/unsubscribeResource() → subscriptions/listen) but does not address the spec's own JSON-RPC response-contract gap. This is the same class of upstream-schema.ts design feedback as #3214351591 (taskId missing), #3214351594 (discriminant not narrowed), and #3216586357 (InputResponse error arm) — concrete enough to block correct SDK implementation, but doesn't independently break CI (the drift guard checks for types that exist, not types that should exist), hence nit.
How to fix
Raise upstream alongside the other schema.ts feedback. Two options:
- Add a response wrapper:
export interface SubscriptionsListenResultResponse extends JSONRPCResultResponse { result: EmptyResult; }and document that the server sends it when the stream closes (or immediately after the ack notification, depending on intended semantics). This matches the deletedSubscribeResultResponse/UnsubscribeResultResponseprecedent. - Document the no-response contract: state explicitly in the
SubscriptionsListenRequestJSDoc that the server MUST NOT send a JSON-RPC response for this request and that clients MUST NOT track itsidin their pending-request map (or, more cleanly, change it to extendJSONRPCNotificationso it carries noidat all — though that loses the ability to error-respond).
Then re-run pnpm run fetch:spec-types. Don't add SubscriptionsListenRequestSchema to schemas.ts until this is settled.
| export interface RequestParams { | ||
| _meta?: RequestMetaObject; | ||
| _meta: RequestMetaObject; | ||
| } |
There was a problem hiding this comment.
🟡 Upstream spec self-inconsistency to batch with the other schema.ts feedback: RequestParams._meta is now required with three required client-identity keys (io.modelcontextprotocol/{protocolVersion,clientInfo,clientCapabilities}), but RequestParams is still referenced from contexts where that doesn't fit — DiscoverRequest.params? and PaginatedRequest.params? are still optional (so a client can type-validly omit the "Required" handshake metadata entirely, and discovery is circular: you must commit to a protocolVersion to ask which versions are supported); the server→client ListTasksRequest inherits PaginatedRequestParams → RequestParams, forcing a server sending tasks/list with a cursor to populate the client's clientInfo/clientCapabilities; and the server-authored ListRootsRequest (now only an InputRequest payload) kept params?: RequestParams while its siblings CreateMessageRequest/ElicitRequest had their RequestParams inheritance stripped. Suggest upstream make the three keys optional (or split RequestMetaObject by direction), make params non-optional on client wire requests, exempt server/discover, and drop the RequestParams reference from ListRootsRequest.
Extended reasoning...
What the issue is
This sync changes RequestParams (line 145) from _meta?: RequestMetaObject to _meta: RequestMetaObject and adds three required keys to RequestMetaObject (lines 82/89/97): 'io.modelcontextprotocol/protocolVersion', 'io.modelcontextprotocol/clientInfo', and 'io.modelcontextprotocol/clientCapabilities', each with JSDoc explicitly saying "Required." The intent — moving the handshake from a one-time initialize to per-request _meta — is clear, but RequestParams is still used in three places where mandatory client-identity metadata is either omittable, semantically wrong, or circular. This is the spec being internally inconsistent, distinct from comment #3223937258 which only notes that the SDK's RequestParamsSchema doesn't match the new required _meta.
Surface 1 — params? still optional on client wire requests
DiscoverRequest.params?: RequestParams (line 568) and PaginatedRequest.params?: PaginatedRequestParams (line 1014, inherited by ListResourcesRequest / ListResourceTemplatesRequest / ListPromptsRequest / ListToolsRequest / ListTasksRequest) declare params as optional. So { jsonrpc: '2.0', id: 1, method: 'tools/list' } is type-valid yet carries no _meta and therefore none of the "Required" handshake metadata — the spec contradicts itself. Pre-sync this was consistent because _meta was optional; the upstream commit tightened _meta without tightening params.
The DiscoverRequest case is additionally circular. Per its JSDoc, server/discover exists so the client can learn supportedVersions for use in subsequent requests, and per the JSDoc on protocolVersion (line 80) the server MUST return UnsupportedProtocolVersionError if the value is not supported. But if the client supplies params, it must include _meta['io.modelcontextprotocol/protocolVersion'] — i.e., commit to a version before learning which versions are supported. Either DiscoverRequest should not use RequestParams, or protocolVersion should be optional/exempt for discovery; the params? escape hatch is at best implicit and contradicts the unconditional "Required." prose.
Surface 2 — server→client wire request inherits client-direction keys
ServerRequest (line 3280) = GetTaskRequest | GetTaskPayloadRequest | ListTasksRequest | CancelTaskRequest. Three of these use inline params: { taskId: string } and escape, but ListTasksRequest (line 2155) extends PaginatedRequest → params?: PaginatedRequestParams (line 1004) extends RequestParams → _meta: RequestMetaObject with required clientInfo/clientCapabilities. The JSDoc on those keys is unambiguously client→server ("Identifies the client software making the request", "The client's capabilities for this specific request", "Servers MUST NOT infer capabilities from prior requests"). So per the type, a server sending tasks/list with a pagination cursor must fabricate the client's identity and capabilities — semantically nonsensical. Concrete SDK consequence: when the companion work adds per-request _meta injection to Protocol.request(), the server-side outbound path has no sensible value to put here.
Surface 3 — server-authored embedded ListRootsRequest payload
ListRootsRequest (line 2820) dropped extends JSONRPCRequest (it is now only an InputRequest payload embedded in server-emitted InputRequiredResult.inputRequests, per InputRequest = CreateMessageRequest | ListRootsRequest | ElicitRequest at line 438), but unlike its two siblings it kept params?: RequestParams (line 2822). The siblings were cleaned: CreateMessageRequestParams (line 2257) and ElicitRequestFormParams/ElicitRequestURLParams (lines 2880/2913) all dropped extends TaskAugmentedRequestParams, so they no longer reference RequestParams/_meta at all. ListRootsRequest was missed — likely because it had no dedicated *Params wrapper to edit. params? is optional so a server can omit it, but a server wanting to attach e.g. _meta.progressToken would be type-forced to also fabricate clientInfo/clientCapabilities/protocolVersion for a server-authored embedded payload.
Step-by-step proof
RequestMetaObject(lines 74-97) declares'io.modelcontextprotocol/protocolVersion': string,'…/clientInfo': Implementation,'…/clientCapabilities': ClientCapabilities— none with?.RequestParams(line 145) declares_meta: RequestMetaObject— no?.PaginatedRequest(line 1014) declaresparams?: PaginatedRequestParams— with?.PaginatedRequestParams extends RequestParams(line 1004).ListToolsRequest extends PaginatedRequest→ a client may send{ jsonrpc:'2.0', id:1, method:'tools/list' }with noparams→ no_meta→ noprotocolVersion/clientInfo/clientCapabilities. Type-valid, but the JSDoc on each key says "Required."ListTasksRequest extends PaginatedRequestand is a member ofServerRequest(line 3280). A server paginatingtasks/listconstructsparams: { cursor: '…', _meta: { 'io.modelcontextprotocol/clientInfo': ???, 'io.modelcontextprotocol/clientCapabilities': ???, … } }— there is no value a server can sensibly put for the client's identity.ListRootsRequest(line 2822) keepsparams?: RequestParams, whileCreateMessageRequest.params: CreateMessageRequestParamsandElicitRequest.params: ElicitRequestParamsno longer extendRequestParams—ListRootsRequestis the odd one out among the threeInputRequestmembers.
Impact
None of these independently break CI beyond what comment #3223937258 already documents (the SDK↔spec RequestParams assignability failure). They are upstream design gaps in the same tier as comments #3214351591 (taskId missing), #3214351594 (discriminant not narrowed), #3216586352 (tasks.requests orphaned), and #3216586357 (InputResponse no error arm) — they don't add new red CI, but they identify shapes the SDK should not encode as-is. In particular, the server-side Protocol.request() _meta-injection redesign called for in #3223937258 cannot follow the same logic as the client side while RequestMetaObject is client-direction-only.
How to fix
Raise upstream alongside the other schema.ts feedback. Plausible fixes (any one resolves all three surfaces, or they can be combined):
- Make the three
io.modelcontextprotocol/*keys optional onRequestMetaObject(or split intoClientRequestMetaObject/ServerRequestMetaObject), and have client→server wire request types makeparamsnon-optional so the handshake metadata is actually carried. - Have
DiscoverRequestuse a params type that does not extendRequestParams(or document an explicit exemption from theprotocolVersionrejection rule forserver/discover). - Have server→client request params (concretely
ListTasksRequestviaPaginatedRequest) not extendRequestParams, or use a server-direction_metashape. - Change
ListRootsRequest.params?to a bare{ _meta?: MetaObject }or dropparamsentirely, matchingCreateMessageRequest/ElicitRequest.
Then re-run pnpm run fetch:spec-types.
There was a problem hiding this comment.
Partially superseded by the latest re-pull (c47bd846), which deletes the entire tasks subsystem.
Surface 2 is now moot: ListTasksRequest and ServerRequest no longer exist (the spec defines no server→client requests at all), so there is no longer a server→client wire request inheriting the client-direction _meta keys via PaginatedRequestParams → RequestParams.
Surfaces 1 and 3 remain fully applicable:
RequestParams._metais still required (line 128) with the three requiredio.modelcontextprotocol/*keys, whileDiscoverRequest.params?(line 549) andPaginatedRequest.params?(line 932) are still optional — a client can still type-validly omit the "Required" handshake metadata, and theserver/discovercircularity (must commit to aprotocolVersionto ask which versions are supported) is unchanged.ListRootsRequest(line 2452) still keepsparams?: RequestParamswhile itsInputRequestsiblingsCreateMessageRequest/ElicitRequesthave had theirRequestParamsinheritance stripped — still the odd one out among server-authored embedded payloads.
The remaining upstream feedback to batch: make the three keys optional (or split RequestMetaObject by direction), make params non-optional on client wire requests / exempt server/discover, and drop the RequestParams reference from ListRootsRequest.
There was a problem hiding this comment.
Updated against the latest re-pull (bbcfc6) — the prior follow-up is now partially stale.
params? optionality (Surface 1, sub-issue) — now resolved upstream: DiscoverRequest.params (line 558) and PaginatedRequest.params (line 941) are now declared required (no ?), so a client can no longer type-validly omit the handshake _meta on those wire requests. This was one of the suggested fixes ("make params non-optional on client wire requests").
Discover circularity (Surface 1, sub-issue) — still applicable, arguably tightened: with DiscoverRequest.params: RequestParams now required, the client must populate _meta['io.modelcontextprotocol/protocolVersion'] to call server/discover at all — i.e. it must commit to a version before learning which versions are supported. The escape hatch is that the server responds with UnsupportedProtocolVersionError.data.supported (now a structured array), so a client can recover by trying a guess and reading the 400 body. Whether that should require an explicit exemption for server/discover remains an upstream design question.
ListRootsRequest (Surface 3) — still applicable: ListRootsRequest (line 2459) still keeps params?: RequestParams while its InputRequest siblings CreateMessageRequest/ElicitRequest had their RequestParams inheritance stripped — still the odd one out among server-authored embedded payloads.
Surface 2 — moot (unchanged from prior follow-up; ListTasksRequest/ServerRequest are gone).
Remaining upstream feedback to batch: exempt server/discover from the protocolVersion requirement (or document the try-and-recover-via-data.supported pattern as the intended bootstrap), and drop the RequestParams reference from ListRootsRequest.
There was a problem hiding this comment.
Updated against the latest re-pull (91b403f8) — most of this comment is now addressed upstream; one sub-issue remains.
ListRootsRequest (Surface 3) — now resolved upstream: ListRootsRequest.params (line 2598-2603) has been changed from params?: RequestParams to a bare params?: { _meta?: MetaObject } — exactly the fix suggested in the original comment ("Change ListRootsRequest.params? to a bare { _meta?: MetaObject } … matching CreateMessageRequest/ElicitRequest"). The server-authored embedded payload no longer references the client-direction RequestParams/RequestMetaObject.
params? optionality (Surface 1, sub-issue) — remains resolved (unchanged from the prior follow-up): DiscoverRequest.params (line 612) and PaginatedRequest.params are required, so the handshake _meta cannot be type-validly omitted on client wire requests.
Discover circularity (Surface 1, sub-issue) — still applicable, now partially mitigated by prose: RequestParams._meta (line 156) still requires the three io.modelcontextprotocol/* keys (lines 83/90/98), and DiscoverRequest.params: RequestParams is required, so a client must still commit to a protocolVersion to call server/discover. The new DiscoverRequest JSDoc (lines 600-603) does soften this — "Clients MAY call it but are not required to — version negotiation can also happen inline via per-request _meta" — and UnsupportedProtocolVersionError.data.supported provides the try-and-recover path, but there is still no explicit exemption from the protocolVersion rejection rule for server/discover itself.
Surface 2 — moot (unchanged; ListTasksRequest/ServerRequest are gone).
Remaining upstream feedback to batch is now narrow: either exempt server/discover from the protocolVersion requirement or document the try-a-version-and-recover-via-data.supported pattern as the intended bootstrap.
29aa626 to
07e2103
Compare
| /** | ||
| * A successful response from the server for a {@link SubscribeRequest | resources/subscribe} request. | ||
| * Sent from the client to open a long-lived channel for receiving notifications | ||
| * outside the context of a specific request. Replaces the previous HTTP GET | ||
| * endpoint and ensures consistent behavior between HTTP and STDIO. | ||
| * | ||
| * @example Subscribe result response | ||
| * {@includeCode ./examples/SubscribeResultResponse/subscribe-result-response.json} | ||
| * @example Listen for tools and resource list changes | ||
| * {@includeCode ./examples/SubscriptionsListenRequest/listen-for-list-changes.json} | ||
| * | ||
| * @category `resources/subscribe` | ||
| * @category `subscriptions/listen` | ||
| */ | ||
| export interface SubscribeResultResponse extends JSONRPCResultResponse { | ||
| result: EmptyResult; | ||
| export interface SubscriptionsListenRequest extends JSONRPCRequest { | ||
| method: 'subscriptions/listen'; | ||
| params: SubscriptionsListenRequestParams; | ||
| } |
There was a problem hiding this comment.
🟡 🟡 The companion redesign also has transport-layer work in packages/{server,client}/src/**/streamableHttp.ts that none of the existing comments mention and that nothing in CI surfaces (those files don't import spec.types.ts — same silent class as the -32042 finding). (1) This JSDoc says subscriptions/listen "Replaces the previous HTTP GET endpoint" — the server transport still implements that endpoint (handleGetRequest / _standaloneSseStreamId='_GET_stream', streamableHttp.ts:234/358/404/433/937/964) and the client transport still opens a bare GET stream for unsolicited notifications (_startOrAuthSse, streamableHttp.ts:233/251/647) instead of issuing subscriptions/listen. (2) The new RequestMetaObject JSDoc (lines 77-80) and the UnsupportedProtocolVersionError/MissingRequiredClientCapabilityError JSDoc add three HTTP MUSTs — _meta protocolVersion MUST match the MCP-Protocol-Version header else 400, and both error envelopes MUST be HTTP 400 — but validateProtocolVersion() (streamableHttp.ts:889) checks only the header (never cross-checks body _meta), and Protocol-layer JSON-RPC errors flow through send() into a hardcoded HTTP 200 (lines 817/1022/1024) with no error-code→HTTP-status hook. The redesign needs to: route subscriptions/listen to the long-lived SSE stream (and decide GET back-compat vs 405); have the client transport send subscriptions/listen instead of opening GET; add header↔_meta cross-validation; and add a Protocol→transport hook so these two error envelopes surface as HTTP 400.
Extended reasoning...
What this finding covers
Two transport-layer normative changes in this sync land in JSDoc prose only and affect packages/server/src/server/streamableHttp.ts and packages/client/src/client/streamableHttp.ts. Neither file imports spec.types.ts, so — like the -32042 / UrlElicitationRequiredError finding (#3216586348) — there is zero CI signal: tsc, spec.types.test.ts, and the specTypeSchema allowlist guard all stay green for these files. None of the 13 existing PR comments mention streamableHttp.ts, the GET handler, or HTTP-status-code mapping; comments #3223937253/#3223937258 cover the schema/lifecycle/subscribeResource() side of subscriptions/listen, and #3231781722 covers its missing JSON-RPC result type, but the transport plumbing is a separate code path.
(1) GET standalone-SSE endpoint replaced by subscriptions/listen
The SubscriptionsListenRequest JSDoc (this line) explicitly says it "Replaces the previous HTTP GET endpoint and ensures consistent behavior between HTTP and STDIO." The Streamable HTTP transport implements that GET endpoint as core functionality:
- Server (
packages/server/src/server/streamableHttp.ts):case 'GET'→handleGetRequest()at lines 358-359/404; the standalone notification stream is keyed by_standaloneSseStreamId = '_GET_stream'(line 234) with 409-conflict handling (line 433), close/cleanup (449/469), and the unsolicited-message send path / event-store replay routed through it (lines 937/964/967). - Client (
packages/client/src/client/streamableHttp.ts):_startOrAuthSse()issuesmethod: 'GET'(lines 233/251) to open the unsolicited-notification stream, kicked off afternotifications/initialized(line 647) and on resume (lines 535/756), with reconnect at 347.
After this sync the spec no longer defines a GET endpoint for the standalone notification stream; a spec-compliant server is not obligated to accept GET. The client transport already tolerates a 405 on GET (lines 285-288), so it would not crash against such a server — but it would silently degrade to never receiving any unsolicited server notifications, because nothing in the client transport sends subscriptions/listen. Conversely, the server transport has no path that routes an incoming subscriptions/listen JSON-RPC request to the long-lived SSE stream — the only way to open that stream is GET.
(2) New HTTP-400 MUSTs with no transport hook
This sync adds three HTTP-transport-specific MUST clauses in JSDoc:
RequestMetaObject['io.modelcontextprotocol/protocolVersion'](lines 77-80): "For the HTTP transport, this value MUST match theMCP-Protocol-Versionheader; otherwise the server MUST return a400 Bad Request."UnsupportedProtocolVersionError(line ~383): "For HTTP, the response status code MUST be400 Bad Request."MissingRequiredClientCapabilityError(line ~413): "For HTTP, the response status code MUST be400 Bad Request."
The server transport's validateProtocolVersion() (streamableHttp.ts:889-898) reads the MCP-Protocol-Version header on every request and 400s if it's not in _supportedProtocolVersions, but it never cross-checks the header against the body params._meta['io.modelcontextprotocol/protocolVersion'] (which the SDK doesn't populate yet — that's the per-request _meta injection work in #3223937258). And Protocol-layer JSON-RPC errors generated by handlers flow back through the transport's send() into an already-opened HTTP 200 SSE/JSON body (lines 817/1022/1024) — there is no hook by which Protocol can tell the transport "this particular JSONRPCErrorResponse should be delivered as HTTP 400 instead of inside a 200." So once the redesign starts emitting UnsupportedProtocolVersionError (INVALID_PARAMS + data.supported/requested) or MissingRequiredClientCapabilityError (-32003), they will go out as HTTP 200, violating the new MUSTs.
Step-by-step proof
GET → subscriptions/listen:
- Spec post-sync:
SubscriptionsListenRequestJSDoc says it "Replaces the previous HTTP GET endpoint"; the GET endpoint is no longer part of the transport spec. - SDK client connects, completes handshake, and at
streamableHttp.ts:647calls_startOrAuthSse({}), which issuesfetch(url, { method: 'GET', headers: { Accept: 'text/event-stream', … } })(line 251). - A spec-compliant server with no GET handler returns 405. Client hits line 287 and silently returns — no error, no notification stream, no
subscriptions/listensent. The user'sToolListChangedNotification/ResourceUpdatedNotificationhandlers never fire. - Conversely, an SDK server receiving a POST
{ method: 'subscriptions/listen', … }would dispatch it throughhandlePostRequest→Protocol._onrequestlike any RPC; nothing wires it to_standaloneSseStreamId, so unsolicitedsend()calls (line 937/967) still look up'_GET_stream'and find nothing.
HTTP-400 MUSTs:
- After the redesign, client sends POST with header
MCP-Protocol-Version: 2026-03-01and bodyparams._meta['io.modelcontextprotocol/protocolVersion'] = '2025-11-07'. validateProtocolVersion()(line 889) reads onlyreq.headers.get('mcp-protocol-version'), sees'2026-03-01'∈_supportedProtocolVersions, returnsundefined. The header↔body mismatch is never checked → no 400, contrary to lines 77-80.- Separately, a tool handler determines the request needs
elicitationbut_meta.clientCapabilities.elicitationis absent, and (per the redesign) emits aMissingRequiredClientCapabilityError(code: -32003).Protocolserialises it and callstransport.send(errorResponse, { relatedRequestId }). send()finds the pending stream for that request and either writes an SSE event into the already-200 stream or callsstream.resolveJson(Response.json(responses[0], { status: 200, … }))(lines 1022/1024). The-32003error reaches the client inside an HTTP 200 body — violating the "MUST be 400" clause.
Why existing code doesn't prevent it
streamableHttp.ts (both packages) does not import spec.types.ts, so the drift guard / spec.types.test.ts cannot see this. The new requirements are JSDoc prose, not types, so even the bidirectional-assignability checks would not catch them. And the existing comments' remediation lists are scoped to schemas.ts/types.ts/client.ts/server.ts — a reviewer following only those could replace subscribeResource() with a subscriptions/listen API and add the two error-envelope schemas without ever touching the transport's GET handler or its hardcoded-200 send path.
Impact
This is forward-looking scoping for the companion redesign rather than an independent blocker (the SDK doesn't yet emit -32003 or populate _meta.protocolVersion, and keeping GET as a back-compat shim is a viable transitional state). But it identifies two non-obvious design decisions the redesign must make at the transport layer — (a) how subscriptions/listen is wired to the long-lived SSE stream and what happens to handleGetRequest, and (b) a new Protocol→transport hook to map specific JSON-RPC error envelopes to non-200 HTTP status — that someone doing pure schemas.ts/server.ts work would plausibly miss.
How to fix
In the companion redesign:
- Server transport: route
method: 'subscriptions/listen'to the long-lived SSE stream (taking over the role of_standaloneSseStreamId/event-store replay); decide whetherhandleGetRequeststays as a back-compat shim or returns 405. ExtendvalidateProtocolVersion()(or add a sibling) to compare theMCP-Protocol-Versionheader against bodyparams._meta['io.modelcontextprotocol/protocolVersion']and 400 on mismatch. Add an error-code→HTTP-status hook sosend()(or the pre-stream path) can emit HTTP 400 when the outgoingJSONRPCErrorResponseis anUnsupportedProtocolVersionError(INVALID_PARAMS+data.supported/requested) orMissingRequiredClientCapabilityError(-32003). - Client transport: replace
_startOrAuthSse's bare GET with a POSTsubscriptions/listencarrying aSubscriptionFilter, and treat the resulting SSE stream (andSubscriptionsAcknowledgedNotification) as the unsolicited-notification channel; keep a GET fallback only if back-compat with pre-spec servers is desired.
| * Capabilities that a server may support. Known capabilities are defined here, in this schema, but this is not a closed set: any server can define its own, additional capabilities. | ||
| * | ||
| * @category `initialize` | ||
| * @category `server/discover` |
There was a problem hiding this comment.
🟡 🟡 Upstream spec self-inconsistency to batch with the other schema.ts feedback (server-side analogue of comment #3216586352): the same diff that deletes SubscribeRequest/UnsubscribeRequest and makes all list-changed notifications opt-in via SubscriptionFilter leaves ServerCapabilities.resources.subscribe (line 776) and {prompts,resources,tools}.listChanged (755/780/795) untouched, and adds no capability flag for subscriptions/listen itself. The flags are not dead — each maps 1:1 to a SubscriptionFilter field — but their documented semantics have silently shifted from "server will push unsolicited *_list_changed" / "server accepts the resources/subscribe RPC" to "server will honor the corresponding SubscriptionFilter opt-in on a subscriptions/listen stream", and the JSDoc/@example references still describe the old model. Suggest upstream either re-document these four flags for the opt-in model or fold them into a structured subscriptions: { listen?, resourceSubscriptions?, …ListChanged? } capability so the SDK's subscriptions/listen client API (replacing the client.ts:599 gate) has an unambiguous flag to check.
Extended reasoning...
What the issue is
This sync replaces the per-resource resources/subscribe / resources/unsubscribe RPCs and unsolicited *_list_changed pushes with a single opt-in subscriptions/listen stream: the client sends a SubscriptionFilter naming which notification types it wants, and the server echoes back what it will honor in SubscriptionsAcknowledgedNotification.params.notifications (the JSDoc on SubscriptionFilter says the server "MUST NOT send notification types the client has not explicitly requested"). But ServerCapabilities — structurally untouched by this diff beyond the @category initialize → server/discover rename at line 721 — still declares:
resources.subscribe?: boolean(line 776) — "Whether this server supports subscribing to resource updates"prompts.listChanged?: boolean/resources.listChanged?: boolean/tools.listChanged?: boolean(lines 755/780/795) — "Whether this server supports notifications for changes to the … list"
and adds no field advertising whether the server implements subscriptions/listen at all.
Addressing the "these flags map cleanly to SubscriptionFilter" objection
A reasonable counter-read is that these four flags are not orphaned: each corresponds 1:1 to a SubscriptionFilter field (resources.subscribe ↔ resourceSubscriptions, {prompts,resources,tools}.listChanged ↔ {prompts,resources,tools}ListChanged), the underlying notifications (ResourceUpdatedNotification, the three *ListChangedNotification types) all still exist in ServerNotification, and the JSDoc text on resources.subscribe ("subscribing to resource updates") is mechanism-agnostic. Under this reading, the static DiscoverResult.capabilities flags tell a client what to put in its SubscriptionFilter before opening a stream, and SubscriptionsAcknowledgedNotification confirms per-stream what was honored — two different purposes, not duplication. That counter-read is largely correct, and is why this is not the same as comment #3216586352's ClientCapabilities.tasks.requests case (where the advertised concept — task-augmented sampling/elicitation wire requests — was deleted entirely).
What that reading does not address is that the semantics of these flags changed without the spec saying so. Pre-sync, tools.listChanged: true meant "this server will send unsolicited notifications/tools/list_changed"; post-sync, per the SubscriptionFilter JSDoc, the server MUST NOT send that notification unless the client opts in on a subscriptions/listen stream — so the flag now means "this server will honor toolsListChanged: true in your filter." Pre-sync, resources.subscribe: true meant "this server accepts resources/subscribe requests" (and client.ts:599 gates on exactly that); post-sync, that RPC does not exist, so the flag must mean "this server will honor resourceSubscriptions: [...] in your filter." Neither shift is reflected in the JSDoc, and the @example {@includeCode ./examples/ServerCapabilities/resources-subscription-*.json} references (lines 763-770) still point at example files written for the old model. An implementer reading ServerCapabilities in isolation would reasonably conclude the server pushes list-changed notifications unprompted and accepts a resources/subscribe RPC — both now wrong.
Separately, there is no explicit capability for subscriptions/listen itself. Under the reinterpretation above, support is implied by any of the four sub-flags being present — but that is undocumented inference, and a server that implements subscriptions/listen for (say) LoggingMessageNotification only, with none of the four sub-flags, has no way to advertise the endpoint exists.
Step-by-step proof
- Diff deletes
SubscribeRequest/UnsubscribeRequest/SubscribeResultResponse/UnsubscribeResultResponse;ClientRequest(line 3256) no longer contains either. - Diff adds
SubscriptionFilter(line 1176) withtoolsListChanged?/promptsListChanged?/resourcesListChanged?/resourceSubscriptions?: string[]and JSDoc "the server MUST NOT send notification types the client has not explicitly requested here."SubscriptionFilter.resourceSubscriptionsJSDoc (line 1194) explicitly says "Replaces the formerresources/subscribeRPC." ServerCapabilitiesbody (lines 723-833) is byte-identical to pre-sync apart from the@categorytag.resources.subscribe(776),prompts.listChanged(755),resources.listChanged(780),tools.listChanged(795) all survive with pre-sync JSDoc. Grep forsubscriptionsunderServerCapabilities→ no match.- Pre-sync semantics of
prompts.listChanged: true: server will sendnotifications/prompts/list_changedwhenever its prompt set changes (no client opt-in required). Post-sync semantics per step 2: server MUST NOT send it unless the client includedpromptsListChanged: truein asubscriptions/listenfilter. The JSDoc at line 753 still reads "Whether this server supports notifications for changes to the prompt list" — silent on the new opt-in requirement. - SDK surface:
packages/client/src/client/client.ts:599doesif (method === 'resources/subscribe' && !this._serverCapabilities.resources.subscribe) throw …. After the redesign already requested in comment #3223937258 that branch is removed, and the newsubscriptions/listenclient API will need a capability gate — under the current spec it must either reuseresources.subscribe/*.listChangedunder their reinterpreted meaning, or ship ungated.
Why this is distinct from existing PR comments
Comment #3216586352 covers the client-side mirror (ClientCapabilities.tasks.requests orphaned by InputRequiredResult). Comment #3223937258 covers the SDK lifecycle redesign, including replacing subscribeResource()/unsubscribeResource() — but that comment is about what the SDK must change, not about the upstream schema.ts leaving ServerCapabilities documenting the old subscription model. Comment #3231781722 covers SubscriptionsListenRequest lacking a JSON-RPC response wrapper. None of #1-#13 mention ServerCapabilities.resources.subscribe / *.listChanged or a missing subscriptions capability.
Impact
No independent CI breakage — all four fields are optional, the ServerCapabilities body is untouched, so spec.types.test.ts's bidirectional check for ServerCapabilities still passes. This is upstream documentation/modeling feedback in the same tier as #3214351593 (JSDoc), #3216586352 (tasks.requests), #3231781731 (RequestParams direction). Filed as nit.
How to fix
Raise upstream alongside the other schema.ts feedback. Two coherent options:
- Re-document in place: keep the four flags, update their JSDoc to reference
SubscriptionFilter(e.g.,resources.subscribe→ "Whether this server will honorresourceSubscriptionson asubscriptions/listenstream";*.listChanged→ "Whether this server will honor*ListChangedon asubscriptions/listenstream"), and refresh the@exampleJSON files. Optionally add a top-levelsubscriptions?: JSONObjectto advertise the endpoint independently of which filter fields are honored. - Restructure: replace the four scattered flags with
subscriptions?: { listen?: JSONObject; resourceSubscriptions?: boolean; toolsListChanged?: boolean; promptsListChanged?: boolean; resourcesListChanged?: boolean }so the capability shape mirrorsSubscriptionFilter.
Then re-run pnpm run fetch:spec-types. Once re-pulled, ServerCapabilitiesSchema in schemas.ts and the client.ts capability gate for the new subscriptions/listen API can follow whichever shape upstream picks.
07e2103 to
0bcec2b
Compare
| export interface DiscoverRequest extends JSONRPCRequest { | ||
| method: 'server/discover'; | ||
| params?: RequestParams; | ||
| } |
There was a problem hiding this comment.
🔴 Another silent transport-layer divergence (same class as #3216586348/#3239359153, distinct surface): the Streamable HTTP server transport's entire Mcp-Session-Id lifecycle is hard-keyed on isInitializeRequest() (packages/server/src/server/streamableHttp.ts:666) — sessionId minting (678), _initialized = true (679), and the public onsessioninitialized hook (683-684) only fire for method: 'initialize'; any other first request hits validateSession() and 400s "Server not initialized" in stateful mode (847-856). After this sync InitializeRequest is gone — a spec-compliant client sends server/discover or just tools/list-with-_meta and never initialize, so a stateful SDK HTTP server 400s every request and never establishes a session. Lines 720-722 also read the deleted initRequest.params.protocolVersion (version now lives in _meta['io.modelcontextprotocol/protocolVersion']). And grepping post-sync spec.types.ts for "session" returns zero matches — the stateless overhaul appears to have dropped Mcp-Session-Id semantics entirely, so the redesign must explicitly decide remove-sessions vs. mint-on-first-request vs. re-key-on-server/discover, and update the public isInitializeRequest guard (guards.ts:93, exports/public/index.ts:111) and sessionIdGenerator/onsessioninitialized/onsessionclosed transport options (streamableHttp.ts:80/89/101) accordingly.
Extended reasoning...
What this finding covers
The Streamable HTTP server transport's session-ID bootstrap is hard-wired to the deleted initialize method, and the post-sync spec appears to have dropped Mcp-Session-Id session semantics entirely. This is a third silent transport-layer divergence in packages/server/src/server/streamableHttp.ts that neither comment #3223937258 (Protocol-layer server.ts/client.ts initialize handling) nor comment #3239359153 (streamableHttp.ts GET→subscriptions/listen + HTTP-400 MUSTs) covers.
The code path
handlePostRequest in packages/server/src/server/streamableHttp.ts:
- Line 666:
const isInitializationRequest = messages.some(element => isInitializeRequest(element))—isInitializeRequest(guards.ts:93) checks formethod: 'initialize'. - Lines 667-685 (the
if (isInitializationRequest)block):this.sessionId = this.sessionIdGenerator?.()(678),this._initialized = true(679), andawait this._onsessioninitialized(this.sessionId)(683-684). This is the only placesessionIdis minted and_initializedis flipped. - Lines 687-694 (the
elsepath): every non-initialize POST goes throughvalidateSession(req). validateSession()(847-856): in stateful mode (sessionIdGeneratorset),if (!this._initialized)→return this.createJsonErrorResponse(400, -32_000, 'Bad Request: Server not initialized').- Lines 720-722:
const initRequest = messages.find(m => isInitializeRequest(m)); const clientProtocolVersion = initRequest ? initRequest.params.protocolVersion : …— readsparams.protocolVersion, a field of the deletedInitializeRequestParams. In the new model the protocol version lives inparams._meta['io.modelcontextprotocol/protocolVersion'].
Why nothing in CI surfaces it
streamableHttp.ts imports isInitializeRequest from @modelcontextprotocol/core (guards.ts), not from spec.types.ts. InitializeRequestSchema still exists in schemas.ts, so guards.ts still typechecks; the transport never references spec.types.ts at all. Neither tsc --noEmit, spec.types.test.ts, nor the specTypeSchema allowlist guard sees this — same silent class as #3216586348 (-32042) and #3239359153 (GET endpoint).
Step-by-step proof
- User constructs the documented stateful server:
new StreamableHTTPServerTransport({ sessionIdGenerator: () => crypto.randomUUID(), onsessioninitialized: id => sessions.set(id, …) })(per the JSDoc example at streamableHttp.ts:195). - A spec-compliant client built against the post-sync schema connects. Per this diff there is no
initializemethod; the client either sends{ method: 'server/discover' }first or goes straight to{ method: 'tools/list', params: { _meta: { 'io.modelcontextprotocol/protocolVersion': …, 'io.modelcontextprotocol/clientInfo': …, 'io.modelcontextprotocol/clientCapabilities': … } } }. handlePostRequestparses the body.isInitializeRequest({ method: 'server/discover', … })→ false (it checks for'initialize').isInitializationRequest = false.- Control falls to line 687-691 →
validateSession(req).sessionIdGeneratoris set,this._initializedis stillfalse(never flipped) → returns 400 "Bad Request: Server not initialized". - The client retries; same result. No request ever succeeds,
sessionIdis never minted,onsessioninitializednever fires, the user's session map stays empty. - Separately, even if a legacy client sent
initialize, line 722 readsinitRequest.params.protocolVersion— fine for legacy clients, but once the redesign re-keys the bootstrap onserver/discover(whoseparams?: RequestParamshas noprotocolVersionfield), this read returnsundefinedand the priming-event protocol-version logic silently picks the wrong branch.
The bigger design question
grep -i session packages/core/src/types/spec.types.ts post-sync → zero matches. The stateless overhaul has apparently dropped Mcp-Session-Id from the protocol entirely (consistent with "every request carries its own _meta handshake"). That means the SDK's public stateful-mode surface — sessionIdGenerator (line 80), onsessioninitialized (89), onsessionclosed (101), the Mcp-Session-Id header read at 859, the DELETE-ends-session path at 838, plus the four middleware READMEs and streamableHttp.examples.ts that demonstrate per-session transport maps keyed on isInitializeRequest — is now unanchored from the spec. The redesign must explicitly choose one of:
- (a) Remove sessions from the transport (stateful mode goes away;
sessionIdGenerator/onsessioninitialized/onsessioncloseddeprecated → breaking change, migration.md entry). - (b) Mint on first request regardless of method (re-key the bootstrap on "first POST without
Mcp-Session-Idheader" instead of "isinitialize"). - (c) Re-key on
server/discover(closest 1:1 swap, butserver/discoveris optional per its JSDoc, so clients that skip it would still 400).
Whichever is chosen, the public isInitializeRequest guard (re-exported at exports/public/index.ts:111, used verbatim in the four middleware READMEs and streamableHttp.examples.ts for per-session routing) needs deprecation or replacement, and lines 720-722 need to read _meta['io.modelcontextprotocol/protocolVersion'] instead of params.protocolVersion.
Why this is distinct from existing comments
- #3223937258 (stateless overhaul) is scoped to the Protocol layer:
client.connect()sendinginitialize,server.tsregistering aninitializehandler /assertInitialized()gating, and the schemas/guards cleanup. Its remediation list never mentions the transport'sisInitializeRequest-gated session bootstrap,sessionIdGenerator,_initialized,onsessioninitialized,validateSession(), or theMcp-Session-Idlifecycle. - #3239359153 is the only existing comment touching
streamableHttp.ts, and it covers exactly two things: (1) GET→subscriptions/listen(handleGetRequest/_standaloneSseStreamId) and (2) the new HTTP-400 MUSTs (validateProtocolVersion()header↔body cross-check, error-code→status hook). It does not mention the session-ID bootstrap path,isInitializationRequest, or theparams.protocolVersionread at 720-722.
A reviewer following only those two comments would rip out assertInitialized() in server.ts and rewire the GET handler — and still ship a stateful HTTP transport that 400s every request from a spec-compliant client.
How to fix
Add to the companion-redesign scope:
- Decide (a)/(b)/(c) above for the
Mcp-Session-Idlifecycle and updatestreamableHttp.ts:664-700+validateSession()accordingly. - Replace the
initRequest.params.protocolVersionread (720-722) withparams._meta['io.modelcontextprotocol/protocolVersion'](or the header, since the bootstrap branch will no longer be exempt from header validation). - Deprecate or repurpose
isInitializeRequest(guards.ts:93, public re-export at exports/public/index.ts:111) and updatepackages/middleware/{node,hono,fastify,express}/README.md+streamableHttp.examples.tswhich use it for per-session transport routing. - If sessions are removed, deprecate
sessionIdGenerator/onsessioninitialized/onsessionclosed(breaking → changeset + migration.md).
0bcec2b to
57ccae0
Compare
e9de0a8 to
786d94c
Compare
| * The desired log level for this request. Optional. | ||
| * | ||
| * If absent, the server MUST NOT send any {@link LoggingMessageNotification | notifications/message} | ||
| * notifications for this request. The client opts in to log messages by | ||
| * explicitly setting a level. Replaces the former `logging/setLevel` RPC. | ||
| */ | ||
| 'io.modelcontextprotocol/logLevel'?: LoggingLevel; |
There was a problem hiding this comment.
🔴 Two new normative MUST-NOT clauses in this sync invert the unsolicited-notification default from opt-out to opt-in — _meta['io.modelcontextprotocol/logLevel'] for notifications/message, and SubscriptionFilter for the four list-changed/resource-updated notifications — and the SDK's server-side default-emit paths now actively violate both, silently (neither server.ts nor mcp.ts imports spec.types.ts, so no CI signal). isMessageIgnored() defaults to send when no level is set, and McpServer auto-fires send*ListChanged() on every registerTool/registerPrompt/registerResource/update gated only on isConnected(), never on a client-supplied filter. The companion redesign needs to invert both defaults (absent level/filter → suppress, not send) and add per-session SubscriptionFilter tracking — neither of which is captured by the existing porting checklists.
Extended reasoning...
What changed in the spec
Two separate JSDoc clauses in this sync flip the protocol's unsolicited-notification model from opt-out to opt-in with normative MUST NOT text:
- Logging (
RequestMetaObject['io.modelcontextprotocol/logLevel'], lines ~99-105): "If absent, the server MUST NOT send anynotifications/messagenotifications for this request. The client opts in to log messages by explicitly setting a level." This replaces the pre-syncLoggingMessageNotificationJSDoc — "If nologging/setLevelrequest has been sent from the client, the server MAY decide which messages to send automatically." MAY-decide → MUST-NOT-without-opt-in. - List-changed / ResourceUpdated (
SubscriptionFilterJSDoc, lines ~1129-1135 and ~1163-1166): "Each notification type is opt-in; the server MUST NOT send notification types the client has not explicitly requested here." The fourSubscriptionFilterfields gate exactlynotifications/{tools,prompts,resources}/list_changedandnotifications/resources/updated. Pre-sync the protocol's model was opt-out (the old*ListChangedNotificationJSDoc says "may be issued by servers without any previous subscription from the client").
What the SDK does — both defaults are still opt-out
Logging (packages/server/src/server/server.ts):
server.ts:194—private _loggingLevels = new Map<string | undefined, LoggingLevel>(), populated only by thelogging/setLevelhandler atserver.ts:142-153. After this sync that RPC is deleted, so a spec-compliant client never populates the map.server.ts:200-203—isMessageIgnored()returnscurrentLevel ? <severity check> : false. With the map empty,currentLevelisundefined, so it returnsfalse("not ignored" = send) for every level on every session.server.ts:646-650—sendLoggingMessage()gates only onthis._capabilities.logging && !isMessageIgnored(...). WithisMessageIgnored()alwaysfalseand the server advertising theloggingcapability, every call emits anotifications/message.server.ts:162—RequestHandlerExtra.log()(the per-request logging helper every tool/prompt/resource handler receives) wires straight through tosendLoggingMessage()with no_meta.logLevelcheck at all.mcp.ts:1024(McpServer.sendLoggingMessage()) is the same path.
List-changed / ResourceUpdated (packages/server/src/server/mcp.ts + server.ts):
mcp.ts:605/621/652/685/747/830/836/996—McpServercallssendResourceListChanged()/sendToolListChanged()/sendPromptListChanged()unconditionally on every tool/prompt/resource register, update, enable, disable, and remove.mcp.ts:1030-1052— those wrappers gate only onisConnected(), nothing else.server.ts:302-340—assertNotificationCapability()checks only the server's ownthis._capabilities.{tools,resources,prompts}advertisement, never the client's per-streamSubscriptionFilteropt-in.grep -rn 'SubscriptionFilter\|subscriptions/listen\|resourceSubscriptions' packages/server/srcreturns zero matches — there is no per-session filter tracking anywhere in the package.
Step-by-step proof
Logging:
- A spec-compliant client built against c47bd846 sends
tools/callwith_meta = { protocolVersion, clientInfo, clientCapabilities }(the three required keys) and nologLevel. - The tool handler calls
extra.log('info', 'starting work'). server.ts:162→sendLoggingMessage()→isMessageIgnored('info', sessionId)→_loggingLevels.get(sessionId)isundefined→ returnsfalse.- The notification is emitted. The client receives an unsolicited
notifications/messagefor a request where it never opted in — violating the new MUST NOT.
List-changed:
- A spec-compliant client connects, never sends
subscriptions/listen, never setstoolsListChanged: true. - Server code does
mcpServer.registerTool('foo', { ... }, handler)— a routine post-connect registration. mcp.ts:836unconditionally callsthis.sendToolListChanged();mcp.ts:1039-1041seesisConnected() === trueand forwards toserver.sendToolListChanged().assertNotificationCapability('notifications/tools/list_changed')atserver.ts:302+passes because the server advertisescapabilities.tools— it never consults a client filter.- The transport emits
notifications/tools/list_changedto a client that never requested it — violating the new MUST NOT. Same for prompts/resources/list_changed and resources/updated.
Why no CI signal & why the existing comments don't catch this
Neither server.ts nor mcp.ts imports spec.types.ts, so the drift guard never trips. This is the same silent-divergence class as #3216586348 (-32042 removal), #3246176899 (session bootstrap), and #3239359153 (GET endpoint) — distinct surface, same blind spot.
The existing comments cover adjacent surfaces but not these defaults: #3223937258 lists logging only as "Replace setLoggingLevel() with a per-request _meta logLevel setter" — a literal port (delete the setLevel handler, read _meta instead) could leave the : false default in isMessageIgnored() untouched and still spam every client. #3239359160 covers stale ServerCapabilities flag JSDoc, #3239359153 covers streamableHttp.ts GET routing, #3246176905 covers the upstream JSDoc contradiction in the *ListChangedNotification types — none mention McpServer's auto-fire-on-registration behavior or that assertNotificationCapability() gates on the wrong side of the connection.
How to fix (companion-redesign scope)
- Logging: Invert
isMessageIgnored()'s default — absent level → suppress. The level source must be the originating request's_meta['io.modelcontextprotocol/logLevel'](threaded throughRequestHandlerExtra.log()), not a per-session map. Delete_loggingLevelsand thelogging/setLevelhandler. Decide whether the un-scopedserver.sendLoggingMessage()/mcpServer.sendLoggingMessage()APIs survive at all — there is no spec-defined channel for unsolicited log notifications post-sync. - List-changed: Add per-session
SubscriptionFiltertracking toServer(or the transport). Havemcp.ts:1030-1052andserver.ts:652-671no-op when the active filter lacks the corresponding field, instead of gating only onisConnected(). Or, transitionally, drop the auto-fire-on-registration behavior and makesendXxxListChanged()an explicit user-called API gated on the discovered filter.
This is forward-looking scoping for the already-required companion redesign on an automated cron PR, not a defect this PR introduces — but it identifies two non-obvious default-direction flips that the existing 22-comment porting checklist would not catch.
786d94c to
edb9e09
Compare
|
|
||
| /** | ||
| * A JSON Schema object defining the expected parameters for the tool. | ||
| * | ||
| * Tool arguments are always JSON objects, so `type: "object"` is required at the root. | ||
| * Beyond that, any JSON Schema 2020-12 keyword may appear alongside `type` — including | ||
| * composition keywords (`oneOf`, `anyOf`, `allOf`, `not`), conditional keywords | ||
| * (`if`/`then`/`else`), reference keywords (`$ref`, `$defs`, `$anchor`), and any other | ||
| * standard validation or annotation keywords. | ||
| * | ||
| * Defaults to JSON Schema 2020-12 when no explicit `$schema` is provided. | ||
| */ | ||
| inputSchema: { | ||
| $schema?: string; | ||
| type: 'object'; | ||
| properties?: { [key: string]: JSONValue }; | ||
| required?: string[]; | ||
| }; | ||
|
|
||
| /** | ||
| * Execution-related properties for this tool. | ||
| */ | ||
| execution?: ToolExecution; | ||
| inputSchema: { $schema?: string; type: 'object'; [key: string]: unknown }; | ||
|
|
||
| /** | ||
| * An optional JSON Schema object defining the structure of the tool's output returned in | ||
| * the structuredContent field of a {@link CallToolResult}. | ||
| * the structuredContent field of a {@link CallToolResult}. This can be any valid JSON Schema 2020-12. | ||
| * | ||
| * Defaults to JSON Schema 2020-12 when no explicit `$schema` is provided. | ||
| * Currently restricted to `type: "object"` at the root level. | ||
| */ | ||
| outputSchema?: { | ||
| $schema?: string; | ||
| type: 'object'; | ||
| properties?: { [key: string]: JSONValue }; | ||
| required?: string[]; | ||
| }; | ||
| outputSchema?: { $schema?: string; [key: string]: unknown }; | ||
|
|
There was a problem hiding this comment.
🔴 Companion-work item: this sync changes Tool.outputSchema from { $schema?, type: 'object', properties?, required? } to { $schema?: string; [key: string]: unknown } — the type: 'object' root requirement is removed and the new JSDoc says "This can be any valid JSON Schema 2020-12." The SDK's ToolSchema in packages/core/src/types/schemas.ts:1326-1333 still hard-codes type: z.literal('object') on outputSchema (with the now-stale JSDoc "Must have type: 'object' at the root level per MCP spec"), so a spec-valid tool advertising e.g. outputSchema: { type: 'string' } or outputSchema: { oneOf: [...] } will be rejected by the SDK's Zod validator. The fix is to relax ToolSchema.outputSchema to accept arbitrary JSON Schema (e.g. z.object({ $schema: z.string().optional() }).catchall(z.unknown()).optional()); inputSchema should keep z.literal('object') since the spec still requires it there.
Extended reasoning...
What changed in the spec
This sync rewrites the Tool.inputSchema and Tool.outputSchema declarations at packages/core/src/types/spec.types.ts:1794-1815:
inputSchemagoes from{ $schema?, type: 'object', properties?, required? }to{ $schema?: string; type: 'object'; [key: string]: unknown }— still requirestype: 'object'at the root (the new JSDoc explicitly says "Tool arguments are always JSON objects, sotype: \"object\"is required at the root"), but opens the rest to arbitrary JSON Schema 2020-12 keywords (composition, conditional,$ref/$defs, etc.).outputSchemagoes from{ $schema?, type: 'object', properties?, required? }to{ $schema?: string; [key: string]: unknown }— thetype: 'object'requirement is removed entirely. The pre-sync JSDoc said "Currently restricted totype: \"object\"at the root level"; the post-sync JSDoc replaces it with "This can be any valid JSON Schema 2020-12." The companion change toCallToolResult.structuredContent(nowunknowninstead of{ [key: string]: unknown }) confirms the intent: structured tool output is no longer required to be an object.
What the SDK still enforces
ToolSchema at packages/core/src/types/schemas.ts:1326-1333 still hard-codes the old restriction on outputSchema:
outputSchema: z
.object({
type: z.literal('object'),
properties: z.record(z.string(), JSONValueSchema).optional(),
required: z.array(z.string()).optional()
})
.catchall(z.unknown())
.optional(),with the JSDoc "Must have type: 'object' at the root level per MCP spec" — that comment is now factually wrong post-sync.
Why existing code does not prevent it
The .catchall(z.unknown()) only permits additional keys beyond the named shape — it does not relax type: z.literal('object'), which is a required key with a literal constraint. Any outputSchema whose root is not an object schema (e.g. { type: 'string' }, { type: 'array', items: ... }, { oneOf: [...] } with no type at all) fails ToolSchema.safeParse() even though it is now spec-valid.
The inputSchema half is not a defect: the spec still requires type: 'object' there, so z.literal('object') is correct, and .catchall(z.unknown()) already permits the new arbitrary-JSON-Schema keywords. The defect is specifically the outputSchema literal.
Step-by-step proof
- After this sync,
Tool.outputSchema?: { $schema?: string; [key: string]: unknown }(spec.types.ts:1814). A tool definition like{ name: 'count', inputSchema: { type: 'object' }, outputSchema: { type: 'integer' } }is a valid specTool. - A spec-compliant server returns this tool in its
tools/listresult. - An SDK client calls
client.listTools().Protocol.request()validates the response withListToolsResultSchema, which containstools: z.array(ToolSchema). ToolSchema.outputSchemarequirestype: z.literal('object'). The tool'soutputSchemahastype: 'integer'. Zod rejects:Invalid literal value, expected "object" at tools[0].outputSchema.type.- The whole
tools/listresponse is rejected and the SDK throws — the spec-valid tool is unusable. - The same happens for tools with
outputSchema: { oneOf: [...] }(notypekey at all) or any non-object root.
Impact
This is the over-strict-Zod-rejecting-spec-valid-payloads class flagged in the repo's Recurring Catches (#1768/#1849/#1169). It is silent — schemas.ts doesn't import spec.types.ts, so neither tsc nor the drift guard's hardcoded type counts surface it directly (the bidirectional assignability check on Tool does fail, but that is buried under the dozens of other failures from this PR).
How to fix (companion work)
Relax ToolSchema.outputSchema to accept arbitrary JSON Schema, e.g.:
outputSchema: z
.object({ $schema: z.string().optional() })
.catchall(z.unknown())
.optional(),and update the JSDoc to drop the "Must have type: 'object'" line. Keep inputSchema.type: z.literal('object') as-is (still spec-required). Also update the CallToolResultSchema.structuredContent field (and ToolResultContent.structuredContent) from z.record(...)/z.object(...) to z.unknown() to match the spec change to structuredContent?: unknown. The output-schema-validation helper in mcp.ts that compiles outputSchema into an Ajv validator and asserts structuredContent is a record will also need to handle non-object schemas.
edb9e09 to
1896892
Compare
| content: ContentBlock[]; | ||
|
|
||
| /** | ||
| * An optional JSON object that represents the structured result of the tool call. | ||
| * An optional JSON value that represents the structured result of the tool call. | ||
| * | ||
| * This can be any JSON value (object, array, string, number, boolean, or null) | ||
| * that conforms to the tool's outputSchema if one is defined. | ||
| */ | ||
| structuredContent?: { [key: string]: unknown }; | ||
| structuredContent?: unknown; | ||
|
|
||
| /** |
There was a problem hiding this comment.
🔴 Companion-work item: this sync widens CallToolResult.structuredContent to unknown and Tool.outputSchema to any JSON Schema 2020-12 root, making false/0/''/null spec-valid structured results — but the SDK gates output validation on JS truthiness rather than presence: packages/server/src/server/mcp.ts:282 (if (!result.structuredContent) → throws InvalidParams), packages/client/src/client/client.ts:877 (if (!result.structuredContent && !result.isError) → throws InvalidRequest), and client.ts:885 (if (result.structuredContent) → silently skips schema validation). All three need to become === undefined / !== undefined presence checks; a literal port that only loosens the Zod schemas (per #3263885960/#3263885954) leaves these intact and re-introduces the rejection at the application layer.
Extended reasoning...
What the bug is
This sync widens CallToolResult.structuredContent (spec.types.ts:1635) from { [key: string]: unknown } to unknown and relaxes Tool.outputSchema (spec.types.ts:1814) to accept any JSON Schema 2020-12 root, with new JSDoc explicitly stating structuredContent "can be any JSON value (object, array, string, number, boolean, or null) that conforms to the tool's outputSchema if one is defined." That means a tool advertising outputSchema: { type: 'boolean' } legitimately returns structuredContent: false, and likewise 0, '', or null for number/string/null schemas.
Where the SDK conflates absent with falsy
Both the server and client output-validation paths gate on JS truthiness of structuredContent, not on !== undefined:
-
Server —
packages/server/src/server/mcp.ts:282invalidateToolOutput():if (!result.structuredContent) { throw new ProtocolError(ProtocolErrorCode.InvalidParams, 'Output validation error: Tool ... has an output schema but no structured content was provided'); }
A handler returning
{ content: [...], structuredContent: false }triggers this throw and the tool call fails — even thoughfalseis a valid result for{ type: 'boolean' }. -
Client —
packages/client/src/client/client.ts:877incallTool():if (!result.structuredContent && !result.isError) { throw new ProtocolError(ProtocolErrorCode.InvalidRequest, 'Tool ... has an output schema but did not return structured content'); }
Same pattern — a spec-compliant server returning
structuredContent: 0causes the SDK client to throw. -
Client —
packages/client/src/client/client.ts:885:if (result.structuredContent) { /* validate against outputSchema */ }
The converse — with a falsy
structuredContentandisError: true, the validation block is silently skipped instead of running, so an error result withstructuredContent: falsebypasses output-schema validation entirely.
Why these checks were correct pre-sync but become wrong post-sync
Pre-sync, structuredContent was typed { [key: string]: unknown } and outputSchema required type: 'object' at the root — the only possible payloads were objects (always truthy) or absent. Truthiness was a perfectly adequate proxy for presence. This sync is what makes falsy primitives spec-valid, so the proxy stops being equivalent and starts rejecting valid wire traffic.
Why a literal port of the schema fix won't catch this
Comments #3263885960 (loosen CallToolResultSchema.structuredContent from z.record to z.unknown() in schemas.ts:1381/1566) and #3263885954 (relax ToolSchema.outputSchema away from z.literal('object')) cover the Zod-shape side. A reviewer who applies only those — loosen the schemas, leave the application-level guards alone — ships a server that throws on every falsy structured result and a client that rejects them, despite both wire types now permitting them. mcp.ts and client.ts don't import spec.types.ts, so neither tsc nor the drift guard surfaces this; the truthiness checks typecheck fine against unknown. There is no runtime test today covering a falsy structured result (it was wire-invalid pre-sync), so CI stays green.
Step-by-step proof (post companion-work that loosens the Zod schemas)
- Server registers a tool with
outputSchema: { type: 'boolean' }whose handler returns{ content: [], structuredContent: false }. tools/listadvertises that schema — spec-valid post-sync.- Client calls
callTool({ name: 'isReady' }). - Server-side:
validateToolOutput()atmcp.ts:282evaluates!false → trueand throwsInvalidParams: 'Tool isReady has an output schema but no structured content was provided'. The tool call fails before reaching schema validation. - Even if the server were fixed: the client receives
{ content: [], structuredContent: false }and atclient.ts:877evaluates!false && !undefined → true, throwingInvalidRequest: 'Tool isReady has an output schema but did not return structured content'. - Same sequence for
structuredContent: 0({ type: 'number' }),''({ type: 'string' }), ornull({ type: 'null' }).
How to fix
Change all three sites to presence checks:
mcp.ts:282→if (result.structuredContent === undefined)client.ts:877→if (result.structuredContent === undefined && !result.isError)client.ts:885→if (result.structuredContent !== undefined)
This preserves the "tool with an outputSchema must return structuredContent" invariant while permitting all falsy JSON values, exactly as the spec now demands. Add a runtime test exercising a falsy structured result (false/0/''/null) so this can't regress.
| * The client's capabilities for this specific request. Required. | ||
| * | ||
| * Capabilities are declared per-request rather than once at initialization; | ||
| * an empty object means the client supports no optional capabilities. | ||
| * Servers MUST NOT infer capabilities from prior requests. | ||
| */ | ||
| 'io.modelcontextprotocol/clientCapabilities': ClientCapabilities; |
There was a problem hiding this comment.
🔴 Companion-work item: this sync adds a normative MUST-NOT clause to RequestMetaObject['io.modelcontextprotocol/clientCapabilities'] — "Capabilities are declared per-request rather than once at initialization … Servers MUST NOT infer capabilities from prior requests" — that the SDK's session-scoped Server._clientCapabilities cache (server.ts:99, written once at server.ts:424 in the now-deleted initialize handler) and the public parameter-less getClientCapabilities()/getClientVersion() getters (server.ts:444-453) cannot satisfy, silently (server.ts does not import spec.types.ts). The companion redesign needs to expose per-request clientCapabilities/clientInfo/protocolVersion on RequestHandlerExtra, deprecate the session-scoped getters (breaking change → migration.md), and rewire the 8 capability-assertion call sites (server.ts:272/279/286/343/414/493/560/568/616) to read from the current request's _meta — none of which is captured by #3223937258's "per-request _meta injection" remediation list, which never names the public getters or the per-request scoping discipline.
Extended reasoning...
What changed in the spec
This sync adds three new required io.modelcontextprotocol/* keys to RequestMetaObject. The JSDoc on 'io.modelcontextprotocol/clientCapabilities' (spec.types.ts:91-97) carries a normative clause:
The client's capabilities for this specific request. Required. Capabilities are declared per-request rather than once at initialization; an empty object means the client supports no optional capabilities. Servers MUST NOT infer capabilities from prior requests.
This is a stronger constraint than "replace the initialize handshake with per-request _meta injection" — it mandates a per-request scoping discipline on the server side: the capability set for any given assertion must come from that request's _meta, never from a cache populated by an earlier request.
What the SDK ships — a session-scoped cache and session-scoped public getters
The SDK's Server class models client capabilities and client identity as a single session-wide value:
Server._clientCapabilities/Server._clientVersion(server.ts:99-100) are private session-scoped fields, written once at server.ts:424-425 inside_oninitialize()— the handler for the (now-deleted)initializerequest.Server.getClientCapabilities()(server.ts:444-446) andServer.getClientVersion()(server.ts:451-453) are public, parameter-less getters whose JSDoc reads "After initialization has completed, this will be populated with the client's reported capabilities." Both return the session-scoped value with no per-request scope. They are consumed bypackages/server/src/experimental/tasks/server.ts:121/217and by user code followingdocs/server.md.- Eight capability-assertion call sites read
this._clientCapabilitiesdirectly: server.ts:272/279/286/343/414/493/560/568/616. None reads from the current request's_meta.
server.ts does not import spec.types.ts, so neither tsc nor spec.types.test.ts flags this — it is the same silent-divergence class as #3216586348 (-32042 removal), #3246176899 (session bootstrap), and #3256562384 (logging/listChanged opt-in defaults).
Why this is not subsumed by #3223937258 (addressing the refutation)
#3223937258's remediation list says: "Replace the connect()-time initialize/initialized handshake with per-request _meta injection (protocolVersion/clientInfo/clientCapabilities) and an optional server/discover call; rip out assertInitialized() gating on the server." That directs the handshake replacement but never names getClientCapabilities()/getClientVersion(), never observes they are session-scoped public APIs, and never calls out that the per-request scoping is normative (a MUST NOT, not just a transport detail). Two concrete things follow that the existing checklist does not capture:
-
The public getters need deprecation/migration regardless of how the rewrite goes. Even after removing the
initializehandler,getClientCapabilities()andgetClientVersion()remain on the public surface with JSDoc that references a deleted handshake. They are parameter-less, so there is no request scope to expose. Either they are removed (breaking →migration.md+ major changeset) or they are deprecated with a pointer at the new per-request API. Neither action is in any of the existing 27 comments. -
A literal port of #3223937258 still violates the MUST NOT. A literal-minded reading of "per-request
_metainjection" is: read each request's_meta.clientCapabilitiesand write it intothis._clientCapabilities. That is still a session-scoped cache that gets refreshed, and (a) under concurrency, two in-flight requests with differentclientCapabilitiescross-contaminate — request 2's assertion can pass on request 1's capabilities — and (b) for a request whose_metaomitsclientCapabilities(e.g. a back-compat path), the stale value from the prior request is read, which is precisely "inferring from prior requests."
Step-by-step proof
- Spec-compliant client sends
tools/call(request A) with_meta['io.modelcontextprotocol/clientCapabilities'] = { sampling: {} }. The tool handler awaitsctx.sample(). - Concurrently (Streamable HTTP, multiple in-flight requests), the same client sends another
tools/call(request B) with_meta['io.modelcontextprotocol/clientCapabilities'] = {}— it has revoked its sampling consent for this request. - After a literal port that re-stamps
this._clientCapabilitiesfrom each request's_meta: request A writes{ sampling: {} }, then request B writes{}. Request A'sctx.sample()then runs the assertion at server.ts:272, readsthis._clientCapabilities?.sampling→undefined→ throwsMcpErroreven though request A did declare sampling. Or, if interleaved the other way, request B's handler callsserver.getClientCapabilities()and sees{ sampling: {} }from request A — the server has "inferred capabilities from a prior request," violating the MUST NOT. - There is no parameter on
getClientCapabilities()to disambiguate which request's capabilities the caller wants, so the public API cannot express the spec's per-request semantics.
Impact
After this sync, Server._clientCapabilities is never populated (the initialize handler has no spec counterpart), so the 8 assertion sites all see undefined and reject every server-initiated sampling/elicitation/roots call — that immediate failure mode is what #3223937258 documents. Once the redesign starts injecting per-request _meta, the deeper problem is that the data model has the wrong shape: a session-scoped cache cannot satisfy a per-request normative MUST NOT, and the public getters cannot expose a per-request value without a new parameter or a new home (e.g., RequestHandlerExtra).
How to fix (companion-redesign scope)
- Expose per-request
clientCapabilities/clientInfo/protocolVersiononRequestHandlerExtraso handlers and capability assertions read the current request's_meta, not a cache. - Deprecate the session-scoped
Server.getClientCapabilities()/Server.getClientVersion()(breaking change →migration.mdentry + changeset). Updatepackages/server/src/experimental/tasks/server.ts:121/217anddocs/server.mdto read from the per-request source. - Rewire the 8 capability-assertion call sites (server.ts:272/279/286/343/414/493/560/568/616) to take the per-request capabilities as a parameter rather than reading
this._clientCapabilities. - Delete
_clientCapabilities/_clientVersiononce nothing reads them.
This is forward-looking scoping for the already-required companion redesign on an automated cron PR, not a defect this PR introduces — but it identifies a concrete public-API breaking change and a normative scoping constraint that the existing 27-comment porting checklist does not name.
1896892 to
b255ba4
Compare
| export const UNSUPPORTED_PROTOCOL_VERSION = -32004; | ||
|
|
||
| /** | ||
| * Returned when the request's protocol version is unknown to the server or | ||
| * unsupported (e.g., a known experimental or draft version the server has | ||
| * chosen not to implement). For HTTP, the response status code MUST be | ||
| * `400 Bad Request`. | ||
| * | ||
| * @internal | ||
| * @example Unsupported protocol version | ||
| * {@includeCode ./examples/UnsupportedProtocolVersionError/unsupported-version.json} | ||
| * | ||
| * @category Errors | ||
| */ | ||
| export interface UnsupportedProtocolVersionError extends Omit<JSONRPCErrorResponse, 'error'> { | ||
| error: Error & { | ||
| code: typeof UNSUPPORTED_PROTOCOL_VERSION; | ||
| data: { | ||
| /** | ||
| * Protocol versions the server supports. The client should choose a | ||
| * mutually supported version from this list and retry. | ||
| */ | ||
| supported: string[]; | ||
| /** | ||
| * The protocol version that was requested by the client. | ||
| */ | ||
| requested: string; | ||
| }; | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 Companion-work item supplementing #3223937253: this revision gives UnsupportedProtocolVersionError its own dedicated constant UNSUPPORTED_PROTOCOL_VERSION = -32004 (replacing the earlier draft's code: typeof INVALID_PARAMS framing that #3223937253 was written against), but packages/core/src/types/enums.ts ProtocolErrorCode has no member for -32004 (alongside the already-flagged missing -32003 and the now-deleted -32042 UrlElicitationRequired member that should be removed). Separately, the Streamable HTTP server transport's existing protocol-version-rejection path at packages/server/src/server/streamableHttp.ts:889-898 (validateProtocolVersion()) already returns HTTP 400 for an unsupported MCP-Protocol-Version header but emits generic code -32_000 with the supported/requested versions only flattened into the message string — wire-divergent from the spec's required -32004 + structured data: { supported: string[]; requested: string } payload, so a spec-compliant client cannot programmatically pick a mutually-supported version and retry.
Extended reasoning...
What changed in the spec
This revision (c47bd846) replaces the earlier draft's UnsupportedProtocolVersionError shape (which used code: typeof INVALID_PARAMS per the existing comment #3223937253) with a dedicated error-code constant:
export const UNSUPPORTED_PROTOCOL_VERSION = -32004;
export interface UnsupportedProtocolVersionError extends Omit<JSONRPCErrorResponse, 'error'> {
error: Error & {
code: typeof UNSUPPORTED_PROTOCOL_VERSION;
data: {
supported: string[];
requested: string;
};
};
}This means the existing comment #3223937253 — which only flags MISSING_REQUIRED_CLIENT_CAPABILITY = -32003 as needing a ProtocolErrorCode member and explicitly characterizes UnsupportedProtocolVersionError as carrying code: INVALID_PARAMS — is now stale on this point and omits a second required constant.
Gap 1: no ProtocolErrorCode member for -32004
packages/core/src/types/enums.ts currently defines:
export enum ProtocolErrorCode {
ParseError = -32_700,
InvalidRequest = -32_600,
MethodNotFound = -32_601,
InvalidParams = -32_602,
InternalError = -32_603,
ResourceNotFound = -32_002,
UrlElicitationRequired = -32_042 // ← spec constant deleted in this same diff
}There is no member for -32003 (already flagged) and no member for -32004 (not yet flagged). The companion redesign that wires UnsupportedProtocolVersionError into the Protocol→transport HTTP-400 hook (per #3239359153) will have no named SDK constant to use, and ProtocolError.fromError() / catch-clause mappings have nothing to switch on.
Gap 2: validateProtocolVersion() is wire-divergent from the new error shape
packages/server/src/server/streamableHttp.ts:889-898 already rejects an unsupported MCP-Protocol-Version header with HTTP 400 — but the JSON body it emits is:
return this.createJsonErrorResponse(400, -32_000, error);
// ^^^^^^^ generic, not -32_004
// where `error` is a string interpolating the supported/requested versionsThe spec's UnsupportedProtocolVersionError envelope requires code: -32_004 and a structured data: { supported: string[]; requested: string } payload so the client can programmatically pick a mutually-supported version and retry. The transport's current response carries the version list only inside the human-readable message string, so a spec-compliant client looking for error.data.supported / error.data.requested gets undefined and would have to regex-parse the message.
createJsonErrorResponse() (streamableHttp.ts:284-288) does already accept an optional data? parameter, but it is typed string — so the fix is a small type-widening (allow an object payload), not a new parameter.
Why nothing in CI surfaces it
Neither enums.ts nor streamableHttp.ts imports spec.types.ts, so neither tsc nor the spec.types.test.ts drift guard sees this — same silent class as #3216586348, #3239359153, and #3246176899.
Step-by-step proof
- Spec post-sync:
UNSUPPORTED_PROTOCOL_VERSION = -32004(spec.types.ts:~370),UnsupportedProtocolVersionError.error.code: typeof UNSUPPORTED_PROTOCOL_VERSIONwithdata: { supported, requested }(spec.types.ts:~383-399). grep -n '32004\|32_004' packages/core/src/types/enums.ts→ no match. The SDK has no name for the new error code.- A client sends a POST with header
MCP-Protocol-Version: 1999-01-01.validateProtocolVersion()at streamableHttp.ts:889 returnscreateJsonErrorResponse(400, -32_000, 'Bad Request: Unsupported protocol version (supported versions: ...)'). - The wire shows
{ error: { code: -32000, message: 'Bad Request: Unsupported protocol version (supported versions: 2025-11-07, 2025-06-18, ...)' } }— nodatafield at all. A spec-compliant client checkingerror.code === -32004doesn't recognise it; checkingerror.data.supportedgetsundefined.
How to fix (companion work)
enums.ts: addProtocolErrorCode.UnsupportedProtocolVersion = -32_004(alongside the already-flaggedMissingRequiredClientCapability = -32_003); remove the staleUrlElicitationRequired = -32_042member whose spec constant was deleted in this same diff (see #3216586348).streamableHttp.ts: changevalidateProtocolVersion()to emitProtocolErrorCode.UnsupportedProtocolVersion(-32_004) and pass a structureddata: { supported: this._supportedProtocolVersions, requested: protocolVersion }payload throughcreateJsonErrorResponse(). WidencreateJsonErrorResponse()'sdataparameter fromstringtostring | object(orunknown).
This is a small supplemental delta on the already-tracked #3223937253 enum-member item and the #3239359153 transport-layer hook, and the entire PR is held pending companion redesign — but the existing comments' INVALID_PARAMS framing is now factually incorrect against the current diff and -32004 is a second distinct constant requiring its own enum member, so worth recording so the companion work doesn't miss it.
There was a problem hiding this comment.
Status update — partially addressed on main:
Gap 1 (enum members) — done. packages/core/src/types/enums.ts now defines ProtocolErrorCode.MissingRequiredClientCapability = -32_003 and ProtocolErrorCode.UnsupportedProtocolVersion = -32_004 (both documented as protocol revision 2026-07-28). The third sub-item — removing UrlElicitationRequired = -32_042 — was evidently decided otherwise: the member is retained, consistent with the deliberately kept 2025-11-25 wire surface (spec.types.2025-11-25.ts snapshot), so no further action is suggested there.
Gap 2 (Streamable HTTP transport) — still open. validateProtocolVersion() in packages/server/src/server/streamableHttp.ts (now ~lines 906-914) still rejects an unsupported MCP-Protocol-Version header with createJsonErrorResponse(400, -32_000, message): generic code -32_000 instead of ProtocolErrorCode.UnsupportedProtocolVersion (-32_004), and the supported/requested versions remain flattened into the message string with no structured data: { supported: string[]; requested: string } payload. createJsonErrorResponse()'s data option is still typed string (line 288), so the type-widening to allow an object payload is also still pending. Nothing under packages/server/src emits -32_004 yet, so a spec-compliant client checking error.code === -32004 / error.data.supported still cannot programmatically pick a mutually-supported version and retry.
The remaining work is the small transport-layer delta: emit -32_004 from validateProtocolVersion(), pass { supported: this._supportedProtocolVersions, requested: protocolVersion } as structured data, and widen createJsonErrorResponse()'s data parameter to accept an object.
There was a problem hiding this comment.
Status update against the latest re-pull (e3f281c7) — still open, and the error-code numbering has shifted under the prior status:
Gap 2 (Streamable HTTP transport) — still open, unchanged. validateProtocolVersion() (packages/server/src/server/streamableHttp.ts:906-913) still rejects an unsupported MCP-Protocol-Version header with createJsonErrorResponse(400, -32_000, message) — generic code, supported/requested versions flattened into the message string, no structured data: { supported, requested } payload — and createJsonErrorResponse()'s data option is still typed string (line 287), so the type-widening is also still pending. Nothing under packages/server/src emits the spec's dedicated unsupported-protocol-version code.
Gap 1 (enum members) — previously marked done, but now stale against this revision. This re-pull renumbers the spec constants: MISSING_REQUIRED_CLIENT_CAPABILITY is now -32021 and UNSUPPORTED_PROTOCOL_VERSION is now -32022 (with a new HEADER_MISMATCH = -32020), under a new partition that reserves -32020..-32099 for spec-defined codes and designates -32000..-32019 as implementation-defined. The members added on main (ProtocolErrorCode.MissingRequiredClientCapability = -32_003, UnsupportedProtocolVersion = -32_004, both documented as protocol revision 2026-07-28) now encode the superseded draft numbering — per the current draft those values carry no spec meaning for 2026-07-28.
Remaining companion work for this thread: renumber (or version-scope) the two ProtocolErrorCode members to -32_021/-32_022, then have validateProtocolVersion() emit UnsupportedProtocolVersion with structured data: { supported: this._supportedProtocolVersions, requested: protocolVersion }, widening createJsonErrorResponse()'s data parameter to accept an object.
b255ba4 to
da9d82e
Compare
5af1602 to
56abf78
Compare
|
|
||
| /** @internal */ | ||
| export const LATEST_PROTOCOL_VERSION = 'DRAFT-2026-v1'; | ||
| export const LATEST_PROTOCOL_VERSION = '2026-07-28'; |
There was a problem hiding this comment.
🔴 Companion-work scoping item: this sync stamps the spec's LATEST_PROTOCOL_VERSION as the concrete release version '2026-07-28' (spec.types.ts:37), but the SDK's own version constants in packages/core/src/types/constants.ts (LATEST_PROTOCOL_VERSION = '2025-11-25', SUPPORTED_PROTOCOL_VERSIONS, DEFAULT_NEGOTIATED_PROTOCOL_VERSION) have no compile-time link to spec.types.ts and nothing in CI surfaces the gap — Protocol._supportedProtocolVersions (shared/protocol.ts:351) and the Streamable HTTP transport's validateProtocolVersion() (streamableHttp.ts:889-898) would 400 any client advertising '2026-07-28', and the data.supported list for the new -32004 UnsupportedProtocolVersionError would come from the same stale constant. The bump (plus the hard-coded assertion at packages/core/test/types.test.ts:24) belongs in the same companion PR that implements the new wire behavior — bumping it alone would falsely advertise support — but none of the existing porting checklists name constants.ts at all.
Extended reasoning...
What this finding covers
This sync changes the spec mirror's LATEST_PROTOCOL_VERSION from the placeholder 'DRAFT-2026-v1' to the concrete release version '2026-07-28' (spec.types.ts:37). The SDK's own version constants live in a separate, hand-maintained file — packages/core/src/types/constants.ts:1-3 — which still declares LATEST_PROTOCOL_VERSION = '2025-11-25', DEFAULT_NEGOTIATED_PROTOCOL_VERSION = '2025-03-26', and SUPPORTED_PROTOCOL_VERSIONS = ['2025-11-25', '2025-06-18', '2025-03-26', '2024-11-05', '2024-10-07']. '2026-07-28' appears nowhere in that list. These constants are public API (re-exported from exports/public/index.ts) and feed two runtime gates: Protocol._supportedProtocolVersions defaults to the list (packages/core/src/shared/protocol.ts:351), and the Streamable HTTP server transport's validateProtocolVersion() (packages/server/src/server/streamableHttp.ts:259, 889-898) rejects any request whose MCP-Protocol-Version header is not in the list with HTTP 400.
Why nothing in CI surfaces it
constants.ts has no compile-time link to spec.types.ts — the only test touching the constant is packages/core/test/types.test.ts:24, which hard-asserts LATEST_PROTOCOL_VERSION === '2025-11-25' against a literal; no test compares the SDK constant to SpecTypes.LATEST_PROTOCOL_VERSION. So unlike the schema/union breakage the drift guard catches, this divergence produces zero red CI — the same silent class as the -32042 removal (#3216586348), the session bootstrap (#3246176899), and the transport-layer items (#3239359153) already accepted on this PR.
Step-by-step proof (post-companion-redesign)
- The companion redesign lands the new wire behavior (per-request
_meta,server/discover,InputRequiredResult, etc.) but the porter follows only the existing 33 comments — none of which nameconstants.ts,SUPPORTED_PROTOCOL_VERSIONS,DEFAULT_NEGOTIATED_PROTOCOL_VERSION, ortypes.test.ts:24. - A spec-compliant client (correctly) sends
MCP-Protocol-Version: 2026-07-28and_meta['io.modelcontextprotocol/protocolVersion'] = '2026-07-28'. validateProtocolVersion()checks the header againstthis._supportedProtocolVersions(defaulted fromSUPPORTED_PROTOCOL_VERSIONS), finds no'2026-07-28', and returns HTTP 400 — every request from a spec-compliant client is rejected even though the SDK now implements the new behavior.- Per the new
RequestMetaObjectJSDoc, the server MUST answer an unknown version with the new-32004UnsupportedProtocolVersionErrorlistingdata.supported— and that supported list would be built from this same stale constant, so the error response itself advertises the wrong set of versions. - Conversely, an SDK client can never negotiate or advertise
'2026-07-28'because its ownLATEST_PROTOCOL_VERSIONis'2025-11-25'.
Addressing the refutation
One reviewer pass argued this reduces to "remember to bump the version constant when implementing the new protocol version," since (a) omitting '2026-07-28' today is correct (the SDK doesn't implement it yet) and (b) any test of the new flow would fail without the bump. Point (a) is true and is exactly how this finding is framed: the bump must land with the companion behavioral redesign, not in this PR and not before — bumping alone would falsely advertise support. Point (b) is weaker than it sounds: the new spec error path is keyed on the server's supported-versions list, so an end-to-end test could exercise the new server/discover/_meta/InputRequiredResult machinery while pinning the test client to '2025-11-25'-era constants and never notice the list is stale; and even where a test does trip, the porter still has to find constants.ts, DEFAULT_NEGOTIATED_PROTOCOL_VERSION, the streamableHttp.ts transport option default (line 259), and the literal assertion at types.test.ts:24 — surfaces no existing comment names. The pre-existing-relationship argument (the spec mirror previously said 'DRAFT-2026-v1', equally absent) misses that this diff is what converts a placeholder draft tag into a concrete, shippable wire version a real client will start advertising — that is the moment the gap becomes consequential.
How to fix (companion-work scope)
In the same companion PR that implements the 2026-07-28 wire behavior: add '2026-07-28' to SUPPORTED_PROTOCOL_VERSIONS and bump LATEST_PROTOCOL_VERSION in packages/core/src/types/constants.ts; decide whether DEFAULT_NEGOTIATED_PROTOCOL_VERSION (still '2025-03-26') should move; update the hard-coded assertion at packages/core/test/types.test.ts:24; and consider adding a test that ties the SDK constant to SpecTypes.LATEST_PROTOCOL_VERSION (or at least to the spec snapshot) so future spec-version bumps stop being silent. Do not bump the constant in this automated sync alone — that would advertise a version the SDK does not yet implement.
0dfe972 to
ef5e88c
Compare
| export type ClientNotification = CancelledNotification | ProgressNotification; | ||
|
|
||
| /** @internal */ | ||
| export type ClientResult = | ||
| | EmptyResult | ||
| | CreateMessageResult | ||
| | ListRootsResult | ||
| | ElicitResult | ||
| | GetTaskResult | ||
| | GetTaskPayloadResult | ||
| | ListTasksResult | ||
| | CancelTaskResult; | ||
| export type ClientResult = EmptyResult; | ||
|
|
||
| /* Server messages */ | ||
| /** @internal */ | ||
| export type ServerRequest = | ||
| | PingRequest | ||
| | CreateMessageRequest | ||
| | ListRootsRequest | ||
| | ElicitRequest | ||
| | GetTaskRequest | ||
| | GetTaskPayloadRequest | ||
| | ListTasksRequest | ||
| | CancelTaskRequest; | ||
|
|
||
| /** @internal */ | ||
| export type ServerNotification = |
There was a problem hiding this comment.
🟡 Upstream schema.ts self-inconsistency to batch with the other feedback on this PR: with ServerRequest deleted entirely (zero server→client requests remain), the rewritten direction unions keep three arms that are unreachable by their own direction semantics — ServerNotification retains CancelledNotification (a server can only cancel a request it issued "in the same direction", and it can issue none), ClientNotification retains ProgressNotification (client→server progress reports on a server-issued request, which no longer exists), and ClientResult = EmptyResult survives as a one-member alias though no message can ever carry a client→server result. Suggest upstream either drop these arms (and delete/never ClientResult) or document them as deliberate forward-compat slots for the SEP-2577 interop window, so the companion ClientNotificationSchema/ServerNotificationSchema/ClientResultSchema work in schemas.ts doesn't bake unreachable shapes into Protocol's direction typing.
Extended reasoning...
What the issue is
This sync deletes export type ServerRequest entirely — grep of the post-sync file returns zero matches, and every remaining extends JSONRPCRequest is client→server (DiscoverRequest:559, PaginatedRequest:961, ReadResourceRequest:1110, SubscriptionsListenRequest:1208, GetPromptRequest:1462, CallToolRequest:1723, CompleteRequest:2450 — all members of ClientRequest). The former server-initiated requests (CreateMessageRequest/ListRootsRequest/ElicitRequest) lost their JSONRPCRequest inheritance and survive only as InputRequest payloads embedded in InputRequiredResult — payload types, not a wire request direction, even under the SEP-2577 12-month retention.
Yet the same diff rewrites the direction unions (lines 3005-3022) to keep three arms whose own semantics presuppose server→client requests:
ServerNotificationincludesCancelledNotification(line 3014).CancelledNotificationParams.requestId"MUST correspond to the ID of a request previously issued in the same direction" (line 520) — so a server→client cancelled notification can only cancel a server-issued request, of which none exist.ClientNotificationincludesProgressNotification(line 3005). Progress flows from the receiver of a request back to the requestor, keyed on aprogressToken"given in the initial request" (line 909). A client→server progress notification reports progress on a request the server issued to the client — which no longer exists. The only conceivable residual use (a client emitting progress against aprogressTokenfound inside an embeddedInputRequest's params) has no spec-defined channel or semantics: input fulfillment is returned viainputResponseson retry, not streamed, and embedded payloads are not issued requests with IDs.ClientResult = EmptyResult(line 3008). A client→server JSON-RPC result exists only as the response to a server→client request. WithServerRequestgone, no message can ever carry aClientResult; the union is retained as a dead one-member alias rather than deleted or markednever.
Step-by-step proof (arm 1; the others are symmetric)
- Post-sync, the only requests in the protocol are the nine
ClientRequestmembers — all client→server. A server holds zero outstanding request IDs of its own issuance. - A server wants to emit
{ method: 'notifications/cancelled', params: { requestId: X } }as aServerNotification. - Per line 520,
XMUST be the ID of a request "previously issued in the same direction" — i.e. server→client. No such request can exist. - Therefore the
ServerNotification.CancelledNotificationarm can never legally appear on the wire. By the same logic,ClientNotification.ProgressNotification(no server-issued request to report progress on) and every value ofClientResult(no server-issued request to respond to) are equally uninhabitable.
The contrast confirms this is a mechanical edit rather than a design choice: the reachable mirror arms are coherent (ClientNotification.CancelledNotification — client cancels its own request; ServerNotification.ProgressNotification — server reports progress on a client request), and the same diff did deliberately prune CreateMessageResult/ListRootsResult/ElicitResult from ClientResult. It pruned union members by deleted-type-name only, leaving the direction-mirror arms of the two type-shared notifications and the now-pointless ClientResult alias behind.
Why this matters for the SDK (silent — no CI signal)
The companion union work will rewrite ClientNotificationSchema/ServerNotificationSchema/ClientResultSchema (schemas.ts ~2095-2110) to mirror the spec. Faithfully encoding these arms bakes unreachable message shapes into the public unions and into Protocol's generic direction typing (SendNotificationT/SendResultT): server-side Protocol would still type-permit sending a notifications/cancelled it can never legally send, and client-side Protocol would keep a result type with no request to respond to. None of the affected runtime files import spec.types.ts, so nothing in CI flags this — spec.types.test.ts's union checks would happily pass once the SDK mirrors the same vestigial arms.
Distinct from existing comments
#3252228068 documents the ServerRequest deletion and states "ClientResult collapses to bare EmptyResult" — but only as deletion inventory; it never observes that the retained ClientResult, the server-direction CancelledNotification arm, or the client-direction ProgressNotification arm are unreachable, and its remediation list does not touch the notification unions. #3246176908 made the unreachability argument for a prior revision's task-only ServerRequest and is resolved/superseded (those types no longer exist). No other comment discusses ClientNotification/ServerNotification arm reachability.
How to fix
Raise upstream with the batched schema.ts feedback: either (a) drop CancelledNotification from ServerNotification, drop ProgressNotification from ClientNotification, and delete ClientResult (or mark it never) until a server→client request direction exists again; or (b) if these are deliberately retained as forward-compat slots for the SEP-2577 deprecated sampling/roots interop window, say so in the union JSDoc so SDK porters know to keep them. Until settled, the companion work should not treat the current union shapes as authoritative for Protocol's direction typing.
There was a problem hiding this comment.
Status update — the latest re-pull (91b403f) addresses two of the three arms; one remains.
-
ClientNotification.ProgressNotification— resolved upstream. The union is nowClientNotification = CancelledNotificationonly (spec.types.2026-07-28.ts:3041); the unreachable client→server progress arm was dropped, exactly as suggested. -
ServerNotification.CancelledNotification— resolved upstream by redefinition rather than removal.CancelledNotificationParams.requestIdis now required and re-documented as "MUST correspond to the ID of a request the client previously issued" (line 569), and the notification's JSDoc (line 582) now gives the server-direction arm an explicit, reachable purpose: on stdio the server sends it solely to terminate asubscriptions/listenstream, referencing the ID of the client-issued listen request, and "Servers MUST NOT use this notification to cancel any other request." The "same direction" wording that made the arm uninhabitable is gone, so this arm is no longer vestigial — note for the companionServerNotificationSchema/Protocol work that the server-side cancel path is now narrowly scoped to stream termination on stdio. -
ClientResult = EmptyResult— still open. Line 3044 still keeps the one-member alias while the protocol defines zero server→client requests (noServerRequest, and every remainingextends JSONRPCRequestis client→server), so no message can ever carry aClientResult. This residual item should stay in the upstreamschema.tsfeedback batch: either deleteClientResult(or mark itnever) until a server→client request direction exists again, or document it as a deliberate forward-compat slot for the SEP-2577 interop window so the companionClientResultSchemawork knows to keep it.
| * - **Prompts**: Unknown prompt name or missing required arguments | ||
| * - **Pagination**: Invalid or expired cursor values | ||
| * - **Logging**: Invalid log level | ||
| * - **Tasks**: Invalid or nonexistent task ID, invalid cursor, or attempting to cancel a task already in a terminal status | ||
| * - **Elicitation**: Server requests an elicitation mode not declared in client capabilities | ||
| * - **Sampling**: Missing tool result or tool results mixed with other content |
There was a problem hiding this comment.
🟡 Nit (upstream prose, batch with the other schema.ts feedback): the InvalidParamsError bullet list still routes "Elicitation: Server requests an elicitation mode not declared in client capabilities" to -32602 (line 323), but the MethodNotFoundError doc rewritten by this same sync (lines 299-301) normatively sends all client-capability misses to the new MissingRequiredClientCapabilityError (-32003) — whose single canonical example is literally missing-elicitation-capability.json. Since elicitation modes are ClientCapabilities.elicitation.{form,url} sub-keys, this bullet directly contradicts the new error's scope; suggest rewriting it upstream to point at MissingRequiredClientCapabilityError so the companion SDK work mapping the server.ts elicitation capability gates to wire codes doesn't have two conflicting spec instructions.
Extended reasoning...
What the issue is
The latest re-pull (upstream commit 14a6e3a2, part of the d6323899→00f8803 delta) leaves spec.types.ts saying three mutually inconsistent things about what error a server returns when a request needs an elicitation capability the client didn't declare:
MethodNotFoundErrordoc (lines 299-301, rewritten by this diff): "A request that requires a client capability the client did not declare is signalled instead by {@link MissingRequiredClientCapabilityError} (-32003)." — i.e. all client-capability misses are normatively routed to-32003.MissingRequiredClientCapabilityError(added by this diff, lines 397-413): "Returned when processing a request requires a capability the client did not declare inclientCapabilities" — and its one canonical example isexamples/MissingRequiredClientCapabilityError/missing-elicitation-capability.json(line 410), i.e. exactly the missing-elicitation-capability case.InvalidParamsErrorbullet list (line 323): still keeps "Elicitation: Server requests an elicitation mode not declared in client capabilities" under-32602.
Elicitation modes are client-capability sub-keys — ClientCapabilities.elicitation = { form?: JSONObject; url?: JSONObject } (spec.types.ts:667-670) — and -32003's data.requiredCapabilities: ClientCapabilities can express the granular requirement ({ elicitation: { url: {} } }). So "an elicitation mode not declared in client capabilities" is by definition "a capability the client did not declare," which per items 1-2 MUST be -32003 — yet item 3 says -32602.
Why this is a partial-migration slip introduced by this sync
Pre-sync the bullet was consistent: no -32003 existed, ElicitRequest was a wire request, and a mode mismatch was a plain params-validation failure. The contradiction is created by this diff, and the upstream commit demonstrably edited both surrounding sites while missing this one: it rewrote the MethodNotFoundError doc two paragraphs above and edited this exact bullet list (the hunk @@ -281,7 +320,6 deletes the Tasks bullet) — but left the sibling elicitation bullet untouched. This is the same missed-sibling pattern as the accepted stale-JSDoc findings on this PR (#3246176905, #3239359160).
The bullet is also dead-letter prose under the new model regardless: post-sync ElicitRequest no longer extends JSONRPCRequest (it is only an InputRequest payload inside InputRequiredResult), so a client cannot return -32602 to an elicitation request at all. The only surviving interpretation — a server-side capability gate rejecting the original tools/call-style request — is precisely -32003's own canonical example.
Step-by-step proof
- Client sends
tools/callwith_meta['io.modelcontextprotocol/clientCapabilities'] = { elicitation: { form: {} } }(form mode only). - The tool handler needs a URL-mode elicitation. The server checks the per-request capabilities and finds
elicitation.urlundeclared. - Per spec.types.ts:299-301 and the
-32003definition + itsmissing-elicitation-capability.jsonexample, the server MUST respond withMissingRequiredClientCapabilityError(-32003,data.requiredCapabilities: { elicitation: { url: {} } }, HTTP 400). - Per spec.types.ts:323, the same scenario ("elicitation mode not declared in client capabilities") is listed as
InvalidParamsError(-32602). - Both passages cannot be followed simultaneously — the spec contradicts itself on the wire code for this exact case.
Concrete SDK consequence
This isn't just doc pedantry: the already-mandated companion work (#3223937253, #3278874075 — add ProtocolErrorCode.MissingRequiredClientCapability = -32003 and emit it on per-request capability misses) has to map the SDK's existing elicitation gates to a wire code. Those gates sit exactly on this fault line: packages/server/src/server/server.ts:220-225 throws SdkError(CapabilityNotSupported) when _clientCapabilities.elicitation is absent, and server.ts:284-292 gates URL mode on _clientCapabilities.elicitation.url. The porter choosing between -32602 (per line 323) and -32003 (per lines 299-301/410) is making a wire-compatibility decision that is breaking to flip later. The example file and the MethodNotFoundError rescope make -32003 clearly the intended answer — the bullet is the stale outlier.
Not a duplicate
The existing -32003 comments (#3223937253: missing enum member/schemas; #3278874075: -32004 transport path and enum members) never mention the InvalidParamsError elicitation bullet or the MethodNotFoundError rescope — the contradicting text only landed upstream in commit 14a6e3a2, pulled after the last detailed review round.
How to fix
Batch with the other upstream schema.ts feedback already on this PR: delete or rewrite the elicitation bullet at spec.types.ts:323 to point at MissingRequiredClientCapabilityError (e.g. move it next to the MethodNotFoundError cross-reference), then re-run pnpm run fetch:spec-types. No change in this repo directly (the file header says DO NOT EDIT). In the companion SDK work, route elicitation-mode capability misses to -32003, matching the canonical example and the MethodNotFoundError doc — not -32602.
| export interface DiscoverResult extends CacheableResult { | ||
| /** | ||
| * The version of the Model Context Protocol that the server wants to use. This may not match the version that the client requested. If the client cannot support this version, it MUST disconnect. | ||
| * MCP Protocol Versions this server supports. The client should choose a | ||
| * version from this list for use in subsequent requests. | ||
| */ | ||
| supportedVersions: string[]; |
There was a problem hiding this comment.
🟡 Scoping correction for two existing checklists: DiscoverResult also extends CacheableResult (spec.types.ts:572), making six extenders total — but #3252228073 enumerates only the five list/read results when directing which schemas should extend CacheableResultSchema, and the #3223937253 DiscoverResultSchema porting item predates the mixin and describes DiscoverResult without ttlMs/cacheScope. The companion port should include DiscoverResultSchema among the CacheableResultSchema extenders, and the upstream ttlMs?/cacheScope? optionality feedback batch should name DiscoverResult too — a required cacheScope: 'public' | 'private' on a capabilities document that can be per-auth/per-tenant is a footgun.
Extended reasoning...
What the gap is
This revision declares export interface DiscoverResult extends CacheableResult at spec.types.ts:572, so the server/discover result requires the new ttlMs: number and cacheScope: 'public' | 'private' fields, exactly like the five list/read results. Grepping the post-sync file confirms there are exactly six CacheableResult extenders:
| Line | Type |
|---|---|
| 572 | DiscoverResult |
| 1029 | ListResourcesResult |
| 1065 | ListResourceTemplatesResult |
| 1123 | ReadResourceResult |
| 1419 | ListPromptsResult |
| 1620 | ListToolsResult |
Neither existing finding records the sixth:
- #3252228073 (the
CacheableResultcomment) enumerates only the five list/read results and its remediation says to have "ListResourcesResultSchema/ListResourceTemplatesResultSchema/ReadResourceResultSchema/ListPromptsResultSchema/ListToolsResultSchemaextend" the newCacheableResultSchema— DiscoverResult is never mentioned. - #3223937253 (the "add
DiscoverRequestSchema/DiscoverResultSchema" porting item, 2026-05-12) was written against revision 8e192a22, beforeCacheableResultexisted (introduced in c47bd846, 2026-05-16). It describesDiscoverResultas{ supportedVersions, capabilities, serverInfo, instructions? }— a field list that is now stale.
The concrete failure path
A porter following both checklists verbatim would (a) build DiscoverResultSchema from #3223937253's stale field list, without ttlMs/cacheScope, and (b) wire CacheableResultSchema into only the five schemas #3252228073 names. The sdkTypeChecks entry for DiscoverResult (which #3223937253 directs the porter to add, and which the coverage check forces) then fails its bidirectional spec = sdk assignability check with TS2741 "Property 'ttlMs' is missing" — plus the AssertExactKeys check once added.
Addressing the refutation
One verifier argued this is fully subsumed: the mixin-level fix "automatically covers DiscoverResult," and the failure is CI-caught, not silent. Both points are partly true but don't dissolve the finding:
- The mixin fix is only automatic if the porter knows DiscoverResultSchema is supposed to extend the mixin. #3252228073's remediation names the five extending schemas explicitly; a porter wiring
CacheableResultSchemainto exactly those five and buildingDiscoverResultSchemafrom #3223937253's pre-mixin field list has followed both checklists to the letter and still mis-ported. The enumeration is the checklist, and it is incomplete. - "CI-caught" applies equally to the accepted #3252228073 itself — every
CacheableResultfinding on this PR is CI-visible, since the drift guard is the whole point of the sync. This PR's established acceptance pattern (#3254078087, #3286031387, #3322449645) treats stale-enumeration/stale-checklist corrections as valid precisely because the comments serve as the porting plan for a held PR; a red TS2741 tells the porter something is missing, not that two prior comments pointed them the wrong way. - The upstream-optionality extension is not subsumed: #3252228073's argument for
ttlMs?/cacheScope?is the HTTPCache-Control-is-opt-in analogy for list/read results. DiscoverResult adds a distinct and stronger argument —ServerCapabilitiescan be dynamic (per-auth, per-tenant), and a requiredcacheScopeforces every server to take a caching stance on its capabilities document, where a careless'public'on a capabilities response served behind auth lets a shared cache leak one tenant's capability surface to another. That belongs in the same upstream feedback batch and is named nowhere else on this PR.
Step-by-step proof
- Porter implements #3223937253:
DiscoverResultSchema = ResultSchema.extend({ supportedVersions: z.array(z.string()), capabilities: ServerCapabilitiesSchema, serverInfo: ImplementationSchema, instructions: z.string().optional() }), and adds thesdkTypeChecks.DiscoverResultentry the comment calls for. - Porter implements #3252228073: adds
CacheableResultSchemaand has the five named list/read schemas extend it. tsc --noEmitonspec.types.test.ts: theDiscoverResultcheck'ssdk = specdirection... actually the failing direction isspec = sdk— assigning the SDK type (nottlMs) toSpecTypes.DiscoverResult(requiresttlMs: number,cacheScope: 'public' | 'private') → TS2741: Property 'ttlMs' is missing in type '{ supportedVersions: string[]; ... }' but required in type 'DiscoverResult'. TheServerResultunion check (which containsDiscoverResult) fails the same way.- Porter must now discover, outside both checklists, that spec.types.ts:572 has an
extends CacheableResultclause that #3252228073's enumeration omitted.
How to fix
No change to this automated sync itself. (1) In the companion port, include DiscoverResultSchema among the CacheableResultSchema extenders — i.e., read #3252228073's "five result types" as six. (2) In the upstream ttlMs?/cacheScope? optionality feedback batch already on the table from #3252228073, explicitly include DiscoverResult, with the added per-auth/per-tenant capabilities-document argument: a required cache directive on server/discover is a footgun where 'public' can leak tenant-specific capability sets through shared caches.
Filed as a nit: this PR is already held for companion work and the gap is CI-visible once the DiscoverResult type check exists — this records the connection neither existing finding names, in the same stale-enumeration-correction tier already established on this PR.
ef5e88c to
bab2413
Compare
| * An InputRequiredResult sent by the server to indicate that additional input is needed | ||
| * before the request can be completed. | ||
| * | ||
| * At least one of `inputRequests` or `requestState` MUST be present. | ||
| * @example InputRequiredResult with elicitation and sampling input requests and request state | ||
| * {@includeCode ./examples/InputRequiredResult/input-required-result-with-elicitation-and-sampling-and-request-state.json} | ||
| * | ||
| * @example InputRequiredResult with request state only (load shedding) | ||
| * {@includeCode ./examples/InputRequiredResult/input-required-result-with-request-state-only.json} | ||
| * | ||
| * @category Multi Round-Trip | ||
| */ | ||
| export interface InputRequiredResult extends Result { | ||
| /* Requests issued by the server that must be complete before the | ||
| * client can retry the original request. | ||
| */ | ||
| inputRequests?: InputRequests; | ||
| /* Request state to be passed back to the server when the client | ||
| * retries the original request. | ||
| * Note: The client must treat this as an opaque blob; it must not | ||
| * interpret it in any way. | ||
| */ | ||
| requestState?: string; | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 Upstream schema.ts nit to batch with the other spec feedback: InputRequiredResult's JSDoc (line 471) says "At least one of inputRequests or requestState MUST be present", but both fields are optional, so { resultType: 'input_required' } with neither field is type-valid yet spec-invalid — and since Result carries an index signature, the degenerate payload is structurally indistinguishable from any other Result. Concrete porting note for the companion InputRequiredResultSchema demanded by #3206453749: a plain looseObject with two .optional() fields would accept the forbidden neither-field payload, and the bidirectional spec↔SDK assignability check can't express the constraint, so it must live in a runtime .refine() (precedent: Base64Schema, schemas.ts:765) unless upstream restructures the type as a two-arm union — the pattern the spec already uses for ElicitRequestParams.
Extended reasoning...
What the issue is
The new InputRequiredResult (spec.types.ts:480-491) declares both of its distinguishing fields optional:
export interface InputRequiredResult extends Result {
inputRequests?: InputRequests;
requestState?: string;
}while its own JSDoc at line 471 is normative: "At least one of inputRequests or requestState MUST be present." So { resultType: 'input_required' } with neither field is type-valid but spec-invalid. And because Result carries [key: string]: unknown and both added fields are optional, the degenerate payload is structurally indistinguishable from any other Result at the type level — compounding (but distinct from) the non-narrowed-resultType finding in #3214351594.
Step-by-step proof
- A server returns
CallToolResultResponsewithresult: { resultType: 'input_required' }— noinputRequests, norequestState. - Against the spec types this assigns cleanly to
InputRequiredResult(both fields optional) and therefore toCallToolResult | InputRequiredResult— TypeScript raises no error. - Per the line-471 MUST, the payload is non-conformant: the client has neither sub-requests to fulfil nor opaque state to replay, so the "retry with
inputResponses/requestState" contract is unsatisfiable. The message is semantically void but type-valid. - The constraint is structurally expressible upstream: the load-shedding example at lines 475-476 (
requestStateonly) confirms each single-field arm is independently valid, so a two-arm union — one arm requiringinputRequests, one requiringrequestState— models it exactly. The spec already uses this pattern forElicitRequestParams = ElicitRequestFormParams | ElicitRequestURLParams. Alternatively the generated schema.json could carryanyOf: [{required: ['inputRequests']}, {required: ['requestState']}]next to the prose.
Concrete SDK consequence (the porting note)
The companion work demanded by #3206453749 must add InputRequiredResultSchema to schemas.ts. The natural encoding — a looseObject with two .optional() fields — accepts the forbidden neither-field payload, so Protocol.request() would validate spec-invalid wire traffic as a well-formed InputRequiredResult. Encoding the constraint as a Zod union on the SDK side instead would break the spec→SDK direction of the bidirectional assignability check in spec.types.test.ts (the spec type admits neither-field; the union does not). So without an upstream fix the MUST can only live in a runtime refinement — .refine(v => v.inputRequests !== undefined || v.requestState !== undefined) — for which the SDK already has precedent (Base64Schema, schemas.ts:765). The drift guard checks shapes, not prose constraints, so nothing in CI points the porter at this.
Addressing the counter-arguments
One review pass argued this is the file's deliberate prose-normative idiom (citing e.g. ElicitResult.content and CancelledNotificationParams.requestId) and that the structural-degeneracy point overlaps #3214351594. On the first point: those cited examples are conditional constraints ("only present when action is accept", "MUST correspond to a prior request ID") that TypeScript genuinely cannot express without dependent types — whereas an at-least-one-of is a plain two-arm union the same file already uses for exactly this shape (ElicitRequestParams), so flagging it is a concrete, actionable upstream suggestion, not pedantry about every prose MUST. On the second: #3214351594 covers the non-narrowed resultType discriminant and mentions structural degeneracy only as a side observation; it never mentions the at-least-one constraint, and neither does any of the other 37 timeline comments (#3206453749 lists InputRequiredResult only as needing a schema; #3216586357 covers the missing InputResponse error arm). The residual .refine() porting note is precisely the kind of checklist item this PR's accepted comment tier exists to record — a literal port following only the existing comments would ship a validator that accepts the forbidden payload.
How to fix
Upstream in schema.ts (batch with the resultType discriminant, missing InputResponse error arm, and the other accepted schema.ts feedback): restructure InputRequiredResult as a two-arm union (one arm with required inputRequests, one with required requestState), or at minimum pair the prose MUST with an anyOf required-arms constraint in the generated schema.json. In the companion SDK work regardless: give InputRequiredResultSchema a .refine() encoding the at-least-one rule, following the Base64Schema precedent.
86d0536 to
fa89096
Compare
0435878 to
4c6b66f
Compare
| */ | ||
| message: string; | ||
|
|
||
| /** | ||
| * The ID of the elicitation, which must be unique within the context of the server. | ||
| * The client MUST treat this ID as an opaque value. | ||
| */ | ||
| elicitationId: string; | ||
|
|
||
| /** | ||
| * The URL that the user should navigate to. | ||
| * |
There was a problem hiding this comment.
🔴 This spec sync deletes the entire URL-elicitation completion surface — the required elicitationId field is removed from ElicitRequestURLParams and ElicitationCompleteNotification is deleted outright (and dropped from ServerNotification) — but the SDK still ships both: ElicitRequestURLParamsSchema (schemas.ts:1994) still requires elicitationId, and the ElicitationComplete schemas/types/ServerNotificationSchema membership all survive. As a result the 2026-07-28 drift guard breaks: the bidirectional check at spec.types.2026-07-28.test.ts:301 fails for ElicitRequestURLParams, and the hard reference to SpecTypes.ElicitationCompleteNotification at lines 361-366 is now an unconditional TS2694 compile error. Companion work needs to drop elicitationId from the draft-spec schema surface and decide retain-for-2025-11-25-snapshot vs. remove for the ElicitationComplete artifacts (this supersedes prior comments #3239359156 and #3371043783, which were written while the notification still existed in the spec).
Extended reasoning...
What changed in the spec
This re-pull (91b403f) removes the correlation/completion machinery for URL-mode elicitation in two coordinated deletions:
ElicitRequestURLParams.elicitationIdis deleted (this hunk, ~lines 2709-2714 of the new file). The URL-elicitation payload is now just{ mode, message, url }— there is no longer a server-assigned ID the client echoes back.ElicitationCompleteNotificationis deleted entirely (the interface and itsnotifications/elicitation/completemethod, ~lines 3023-3028 pre-diff) and removed from theServerNotificationunion. A grep of the post-syncspec.types.2026-07-28.tsreturns zero occurrences of eitherelicitationIdorElicitationCompleteNotification.
These are two halves of one surface: elicitationId existed solely so the (now-deleted) completion notification could reference which out-of-band elicitation finished.
What the SDK still ships
packages/core/src/types/schemas.ts:1994—ElicitRequestURLParamsSchemastill declareselicitationId: z.string()as a required field (andElicitationCompleteNotificationParamsSchemaat schemas.ts:2021/2025 also requires it).schemas.ts:2021-2036—ElicitationCompleteNotificationParamsSchema/ElicitationCompleteNotificationSchemastill exist, andElicitationCompleteNotificationSchemais still a member ofServerNotificationSchema(schemas.ts:2244).types.ts:374-375— the inferred type aliases survive;specTypeSchema.ts:63-64— both schemas remain registered in the spec-schema allowlist.
How it breaks the drift guard
packages/core/test/spec.types.2026-07-28.test.ts is the drift guard for this snapshot:
- TS2694 compile error. Lines 361-366 hard-reference
SpecTypes.ElicitationCompleteNotificationin thesdkTypeChecksmap. With the export deleted, this is an unconditional "Namespace has no exported member" error — the test file no longer compiles against this sync. ElicitRequestURLParamsbidirectional check fails. Line 301 runssdk = spec; spec = sdk;forElicitRequestURLParams, and this type is not in theMISSING_SDK_TYPES_2026_07_28allowlist. Thesdk = specdirection now fails with TS2741: the spec value lacks theelicitationIdthe SDK type requires.
(One nuance: ServerNotification itself is in MISSING_SDK_TYPES_2026_07_28, so the union mismatch does not surface as a separate test failure — the breakage above is the concrete CI signal.)
Step-by-step proof
- Spec post-sync:
ElicitRequestURLParams = { mode: 'url'; message: string; url: string }— noelicitationId. - SDK:
ElicitRequestURLParamsSchemainfers{ mode: 'url'; message: string; url: string; elicitationId: string; ... }withelicitationIdrequired. spec.types.2026-07-28.test.ts:301doessdk = spec→ TS2741 (elicitationIdmissing in spec type but required in SDK type).- Independently, lines 361-363 reference
SpecTypes.ElicitationCompleteNotification→ TS2694, since the export no longer exists. - At runtime, a draft-spec-valid URL-elicitation
InputRequestpayload omittingelicitationIdis rejected by the SDK's Zod validator — the over-strict-Zod class this repo has hit before (fix(core): allow additional JSON Schema properties in elicitInput requestedSchema #1768/fix(core): allow extra JSON Schema keys in elicit primitive schemas #1849/fix: allow any JSON Schema type for outputSchema #1169).
Why this isn't covered by existing comments
Prior comment #3239359156 flagged the delivery-channel gap for ElicitationCompleteNotification while the type still existed in the spec — the deletion makes that moot. #3371043783 flagged only a missing Params test entry on the assumption "the SDK side is already complete" — the situation has inverted: the spec dropped the type, and the SDK artifacts are now the leftover, not the gap. No existing comment notes the elicitationId field removal at all.
How to fix (companion work)
- Drop
elicitationIdfromElicitRequestURLParamsSchemafor the draft/2026-07-28 surface (or scope the requirement to the 2025-11-25 snapshot, where the field still exists). - Decide retain-vs-remove for the ElicitationComplete schemas: the 2025-11-25 snapshot (
spec.types.2025-11-25.ts) still defines the notification, so the likely answer is keep the SDK schemas for that interop window but remove/relocate thesdkTypeChecksentry atspec.types.2026-07-28.test.ts:361-366(and adjust theElicitRequestURLParamsentry at line 301) so the 2026-07-28 drift guard compiles and passes. - Update the
ServerNotificationSchemamembership decision in line with whichever spec revision the SDK union is meant to track.
| */ | ||
| export const UNSUPPORTED_PROTOCOL_VERSION = -32004; | ||
|
|
||
| /** | ||
| * Returned when a server rejects a request because the values in the HTTP | ||
| * headers do not match the corresponding values in the request body, or | ||
| * because required headers are missing or malformed. For HTTP, the response | ||
| * status code MUST be `400 Bad Request`. | ||
| * | ||
| * @example Header mismatch | ||
| * {@includeCode ./examples/HeaderMismatchError/header-mismatch.json} | ||
| * | ||
| * @category Errors | ||
| */ | ||
| export interface HeaderMismatchError extends Omit<JSONRPCErrorResponse, 'error'> { | ||
| error: Error & { | ||
| code: typeof HEADER_MISMATCH; | ||
| }; |
There was a problem hiding this comment.
🔴 The spec now reserves -32001 for the new HeaderMismatchError (HTTP headers don't match the request body / required headers missing — HTTP status MUST be 400), but the Streamable HTTP server transport already emits -32_001 on the wire with different semantics: createJsonErrorResponse(404, -32_001, 'Session not found') at packages/server/src/server/streamableHttp.ts:887 (asserted by server and middleware tests), so a 2026-07-28-compliant client switching on error.code === -32001 will misread the SDK's session-expiry response. The companion work needs to either move session-not-found to a different implementation-specific code or adopt the spec semantics, and also add the missing SDK surface — ProtocolErrorCode has no member for -32001, there is no HeaderMismatchErrorSchema/types alias/specTypeSchema registration, and the new HeaderMismatchError export trips the spec.types.2026-07-28.test.ts coverage check.
Extended reasoning...
What changed in the spec
This sync adds a dedicated error code and envelope for header/body mismatches on the Streamable HTTP transport (spec.types.2026-07-28.ts:390 and ~408-422):
export const HEADER_MISMATCH = -32001;
export interface HeaderMismatchError extends Omit<JSONRPCErrorResponse, 'error'> {
error: Error & {
code: typeof HEADER_MISMATCH;
};
}The JSDoc is normative: it is returned "when a server rejects a request because the values in the HTTP headers do not match the corresponding values in the request body, or because required headers are missing or malformed", and "For HTTP, the response status code MUST be 400 Bad Request."
Gap 1 — wire-code collision with the SDK's existing -32001 usage
The Streamable HTTP server transport already emits this exact numeric code with completely different semantics. packages/server/src/server/streamableHttp.ts:887 returns:
return this.createJsonErrorResponse(404, -32_001, 'Session not found');i.e. HTTP 404 + code -32001 = "session expired/not found", and that pairing is pinned by tests (packages/server/test/server/streamableHttp.test.ts:292, packages/middleware/node/test/streamableHttp.test.ts:538 and :934). Until now -32001 was an unclaimed implementation-defined code, so this was fine. Once 2026-07-28 ships, the spec owns -32001 with "header mismatch, MUST be 400" semantics.
Step-by-step failure scenario:
- A 2026-07-28-compliant client implements the spec's error handling: on
error.code === -32001it treats the failure as a header/body mismatch (e.g. it re-derives itsMCP-Protocol-Versionheader orx-mcp-header-mirrored values from the body and retries the same session). - That client talks to an SDK server in stateful mode. Its session expires (server restart, eviction). It sends a request with the stale
Mcp-Session-Id. - The SDK transport replies HTTP 404 with body
{ error: { code: -32001, message: 'Session not found' } }. - The client's spec-driven decode path classifies this as a header mismatch rather than session loss, so it does not perform the correct recovery (re-initialize / start a new session); it may retry the same dead session indefinitely or surface a misleading error. The HTTP status disagreement (404 observed vs the spec's "MUST be 400") makes the response doubly inconsistent for any client that validates both.
Why nothing prevents this: streamableHttp.ts does not import spec.types.*, the -32_001 literal is hand-written, and no test compares the transport's implementation-specific codes against the spec's reserved range — so the collision is silent in CI.
Fix direction (companion work decision): either re-code session-not-found to a code the spec has not claimed (keeping the 404), or adopt the spec's -32001/400 semantics for actual header-mismatch conditions and use a different code for session loss. Either way the two server/middleware tests that pin 404 + -32001 + 'Session not found' need updating in the same change.
Gap 2 — missing SDK surface and drift-guard failure
HeaderMismatchError / HEADER_MISMATCH have no SDK counterpart anywhere outside the spec snapshot:
packages/core/src/types/enums.tsProtocolErrorCodehas members for-32_002(ResourceNotFound),-32_003(MissingRequiredClientCapability),-32_004(UnsupportedProtocolVersion), and-32_042, but no member for-32_001— so the companion transport hook that is supposed to emit this error (e.g. when the body_metaprotocol version disagrees with theMCP-Protocol-Versionheader, or when anx-mcp-header-mirrored argument disagrees with the body) has no named constant to use.- A grep for
HeaderMismatchacrosspackages/corematches onlyspec.types.2026-07-28.ts— noHeaderMismatchErrorSchemainschemas.ts, notypes.tsalias, nospecTypeSchemaregistration. - The drift guard is CI-visible:
packages/core/test/spec.types.2026-07-28.test.tsextracts every exported type, andHeaderMismatchErroris in neithersdkTypeChecksnor theMISSING_SDK_TYPES_2026_07_28allowlist (which namesMissingRequiredClientCapabilityErrorandUnsupportedProtocolVersionErrorbut not this one), so the "should have comprehensive compatibility tests" check fails on this name, and the new export also contributes to the type-count mismatch againstexpect(specTypes).toHaveLength(150).
Why this isn't covered by existing comments
None of the existing PR comments mention -32001 or HeaderMismatchError — the error-code comments on this PR cover only -32003/-32004 (and the prior header/body cross-check comment #3239359153 predates this dedicated code and pointed at UnsupportedProtocolVersionError instead). The session-not-found collision in particular is a new wire-compatibility decision that no existing checklist names.
How to fix (companion-work scope)
- Decide the
-32001collision: re-code the transport's session-not-found response, or align it with the spec's header-mismatch semantics; updatepackages/server/test/server/streamableHttp.test.ts:292andpackages/middleware/node/test/streamableHttp.test.ts:538/934accordingly. - Add
ProtocolErrorCode.HeaderMismatch = -32_001toenums.ts(mirroring the recently added-32_003/-32_004members), plus aHeaderMismatchErrorSchema/types alias/specTypeSchemaregistration, or addHeaderMismatchErrortoMISSING_SDK_TYPES_2026_07_28if the SDK chooses to model it as an error class instead. - Wire the transport-side emission point: when headers and body disagree (e.g.
MCP-Protocol-Versionheader vs_meta['io.modelcontextprotocol/protocolVersion'], orx-mcp-headermirrors), emit-32001with HTTP 400 per the new MUST.
There was a problem hiding this comment.
Status update — the latest re-pull resolves the wire-code collision (Gap 1) upstream; the SDK-surface gap (Gap 2) remains, with renumbered codes.
Gap 1 (collision with the SDK's -32001 session-not-found) — resolved upstream. The spec no longer claims -32001: HEADER_MISMATCH is now -32020 (spec.types.2026-07-28.ts:410), and the new error-code partitioning comment (lines ~383-401) explicitly reserves -32000 to -32019 as implementation-defined — "the specification will never define codes in this sub-range". The Streamable HTTP transport's createJsonErrorResponse(404, -32_001, 'Session not found') (packages/server/src/server/streamableHttp.ts:887) is therefore no longer in conflict, and no re-coding of session-not-found (or the server/middleware tests that pin it) is needed.
Gap 2 (missing SDK surface + drift guard) — still open, now at -32020.
ProtocolErrorCode(packages/core/src/types/enums.ts) still has no member for the header-mismatch code; there is still noHeaderMismatchErrorSchema/ types alias /specTypeSchemaregistration (a grep forHeaderMismatchmatches only the spec snapshot).HeaderMismatchErroris in neithersdkTypeChecksnorMISSING_SDK_TYPES_2026_07_28inpackages/core/test/spec.types.2026-07-28.test.ts, so the "should have comprehensive compatibility tests" check still fails on this name (and it still contributes to thetoHaveLength(150)count mismatch).- The transport-side emission point (header↔body mismatch →
-32020with HTTP 400) is still unimplemented.
New wrinkle from the same renumbering: the spec also moved MISSING_REQUIRED_CLIENT_CAPABILITY to -32021 and UNSUPPORTED_PROTOCOL_VERSION to -32022. The recently added ProtocolErrorCode.MissingRequiredClientCapability = -32_003 and UnsupportedProtocolVersion = -32_004 (enums.ts:19/24, documented as protocol revision 2026-07-28) are now stale against this revision — the partitioning note says only -32002 and -32042 remain reserved from earlier revisions. The companion work should re-key those members (or version them) alongside adding HeaderMismatch = -32_020, rather than mirroring the old -32003/-32004 values as this comment originally suggested.
| /** | ||
| * Extends {@link MetaObject} with additional notification-specific fields. All key naming rules from `MetaObject` apply. | ||
| * | ||
| * @see {@link MetaObject} for key naming rules and reserved prefixes. | ||
| * @see [General fields: `_meta`](/specification/draft/basic/index#meta) for more details. | ||
| * @category Common Types | ||
| */ | ||
| export interface NotificationMetaObject extends MetaObject { | ||
| /** | ||
| * Identifies the subscription stream a notification was delivered on. The | ||
| * server MUST include this key on every notification delivered via a | ||
| * {@link SubscriptionsListenRequest | subscriptions/listen} stream, so the | ||
| * client can correlate the notification with the originating subscription. | ||
| * The key is absent on notifications not delivered via a subscription | ||
| * stream (e.g. progress notifications for an in-flight request), which is | ||
| * why it is optional here. | ||
| * | ||
| * The value is the JSON-RPC ID of the `subscriptions/listen` request that | ||
| * opened the stream. | ||
| */ | ||
| 'io.modelcontextprotocol/subscriptionId'?: RequestId; | ||
| } |
There was a problem hiding this comment.
🔴 This sync adds a new exported NotificationMetaObject interface (carrying the reserved io.modelcontextprotocol/subscriptionId meta key) and changes NotificationParams._meta from MetaObject to it, but the SDK has no NotificationMetaObjectSchema and the drift-guard test has neither an sdkTypeChecks entry nor a MISSING_SDK_TYPES_2026_07_28 allowlist entry for the new name — so the coverage check and the hardcoded export-count assertion in spec.types.2026-07-28.test.ts both fail. The companion port also needs to note that notifications now require their own meta shape (distinct from both request and result meta), which extends the two-way RequestMetaSchema/MetaObjectSchema split prescribed in #3278874070 into a three-way split, and that the eventual subscriptions/listen server implementation MUST stamp subscriptionId on every stream-delivered notification.
Extended reasoning...
What changed
This re-pull adds export interface NotificationMetaObject extends MetaObject at packages/core/src/types/spec.types.2026-07-28.ts:120, defining the reserved key 'io.modelcontextprotocol/subscriptionId'?: RequestId — the JSON-RPC ID of the subscriptions/listen request that opened the stream, which the server MUST include on every notification delivered via that stream (and omit on notifications not delivered via a subscription stream, e.g. progress notifications for an in-flight request). The same hunk changes NotificationParams._meta from MetaObject to NotificationMetaObject (spec.types.2026-07-28.ts:173).
Why the drift guard goes red on this name
packages/core/test/spec.types.2026-07-28.test.ts extracts every export interface|class|type name from the snapshot and asserts each one not in MISSING_SDK_TYPES_2026_07_28 (lines 381-491) has an sdkTypeChecks entry; it also hardcodes expect(specTypes).toHaveLength(150) at line 521. NotificationMetaObject appears nowhere in the test file (grep: 0 matches), so:
- Coverage check —
should have comprehensive compatibility testsfails withNotificationMetaObjectinmissingTests(line 539'stoHaveLength(0)). - Type count — the file now has 151 exports (
grep -cE '^export (interface|class|type) '= 151), so thetoHaveLength(150)assertion at line 521 fails.
Step-by-step: (1) extractExportedTypes picks up 'NotificationMetaObject' from line 120; (2) the allowlist at lines 381-491 does not contain it; (3) sdkTypeChecks['NotificationMetaObject'] is undefined → the name lands in missingTests → assertion fails; (4) independently, 151 ≠ 150 fails the count check. None of the existing porting checklists on this PR (#3223937253's additive batch, #3206453749, #3252228073, or the later corrections) enumerate this export, so a porter working through them would still have a red coverage check on this one name.
Why the SDK side is also a real gap, not just a test-file addition
A grep across packages/core/src finds NotificationMetaObject and io.modelcontextprotocol/subscriptionId only in the spec snapshot — there is no NotificationMetaObjectSchema in schemas.ts, no types.ts alias, and no SUBSCRIPTION_ID_META_KEY constant. NotificationsParamsSchema._meta currently reuses RequestMetaSchema (schemas.ts:110), and the test file does have an sdkTypeChecks.NotificationParams bidirectional entry (test line 77), so the spec-side change of NotificationParams._meta makes that check relevant: the notification meta shape now carries an optional RequestId-typed reserved key the SDK schema does not model.
This also invalidates the prescription in #3278874070, which directed the companion port to split the shared RequestMetaSchema two ways — a request-direction RequestMetaSchema for BaseRequestParamsSchema and a bare MetaObjectSchema for ResultSchema and NotificationsParamsSchema. After this revision, notifications need their own meta shape (bare MetaObject plus the optional subscriptionId key), distinct from both the request meta and the bare result meta — i.e. a three-way split (RequestMetaSchema / NotificationMetaObjectSchema / MetaObjectSchema), not the two-way one the existing comment prescribes. A porter following #3278874070 verbatim would wire NotificationsParamsSchema._meta to the bare MetaObjectSchema and the bidirectional/exact-keys checks would still need the subscriptionId-bearing shape.
Companion-work scoping (server side)
The MUST-include rule is a server-side delivery requirement: when the eventual subscriptions/listen implementation lands (per the existing transport-layer comments #3239359153/#3246176899), every notification routed onto a subscription stream must be stamped with _meta['io.modelcontextprotocol/subscriptionId'] = <id of the listen request>. No notification-emit path in packages/server stamps any subscription correlation key today, and no existing porting checklist names this requirement.
How to fix
In the companion PR (this automated sync is already held for that work): add NotificationMetaObjectSchema to schemas.ts (bare loose meta object plus optional 'io.modelcontextprotocol/subscriptionId': RequestIdSchema), point NotificationsParamsSchema._meta at it (adjusting the #3278874070 split to three-way), export the SUBSCRIPTION_ID_META_KEY constant, add the sdkTypeChecks.NotificationMetaObject entry (or an allowlist entry if the SDK chooses not to model it), bump the toHaveLength count once all deltas are reconciled, and include the stamp-subscriptionId-on-stream-delivery requirement in the subscriptions/listen server implementation's scope.
| * Property schemas may carry an `x-mcp-header` annotation to mirror the | ||
| * argument value into an HTTP header on the Streamable HTTP transport. See | ||
| * the Streamable HTTP transport specification for the validity and | ||
| * extraction rules. | ||
| * | ||
| * Defaults to JSON Schema 2020-12 when no explicit `$schema` is provided. |
There was a problem hiding this comment.
🟡 Companion-work scoping: this revision adds JSDoc to Tool.inputSchema (spec.types.2026-07-28.ts:1884-1888) introducing the x-mcp-header annotation, which mirrors a tool argument value into an HTTP header on the Streamable HTTP transport — but neither packages/client/src/client/streamableHttp.ts nor packages/server/src/server/streamableHttp.ts implements any argument-to-header mirroring or extraction/validation, and the feature is not named in any of the existing companion-work comments on this PR. Recording it here so the eventual 2026-07-28 transport port doesn't miss this transport-level feature (silent: neither transport file imports the spec snapshot, so there is no CI signal).
Extended reasoning...
What changed in the spec
This sync adds new JSDoc prose to Tool.inputSchema (packages/core/src/types/spec.types.2026-07-28.ts:1884-1888): "Property schemas may carry an x-mcp-header annotation to mirror the argument value into an HTTP header on the Streamable HTTP transport. See the Streamable HTTP transport specification for the validity and extraction rules." This is a transport-level feature of the 2026-07-28 revision: a tool author annotates a property in inputSchema, the client transport mirrors that argument's value into an HTTP header on the tools/call POST, and the server transport extracts/validates it against the body — backed by the new HEADER_MISMATCH = -32001 / HeaderMismatchError envelope this same diff introduces ("the values in the HTTP headers do not match the corresponding values in the request body" → HTTP 400).
Why neither transport supports it
A grep for x-mcp-header across packages/ matches only this spec snapshot. On the client side, packages/client/src/client/streamableHttp.ts builds POST headers exclusively from _commonHeaders() / auth / mcp-session-id / mcp-protocol-version / requestInit.headers (≈ lines 213-253 and the send() path at ≈ 543-552); it never inspects the outgoing tools/call arguments, never consults the tool's inputSchema, and has no notion of per-argument header mirroring. On the server side, packages/server/src/server/streamableHttp.ts has no path that extracts header values for x-mcp-header-annotated arguments or cross-validates them against the request body — and its only -32001 usage today is the unrelated "Session not found" response, so there is no HeaderMismatch plumbing either (the -32001 code collision itself is tracked by a separate finding; this comment is about the argument-mirroring feature, a distinct surface).
Why this is silent and not covered by the existing checklist
This is the same silent-transport-divergence class already accepted repeatedly on this PR (#3239359153 GET→subscriptions/listen + HTTP-400 MUSTs, #3246176899 session bootstrap, #3256562384 opt-in notification defaults): the requirement lives in JSDoc prose, not types, and neither transport file imports spec.types.ts, so tsc, spec.types.test.ts, and the specTypeSchema allowlist guard all stay green for these files. None of the 40+ existing comments mention x-mcp-header — the closest transport-layer comments (#3239359153, #3246176899) cover the GET endpoint, protocol-version header validation, and session bootstrap, not argument-to-header mirroring. A porter working through the existing checklist could complete every enumerated transport item and still ship a Streamable HTTP transport pair with no support for this 2026-07-28 feature.
Step-by-step proof
- A tool author registers a tool whose
inputSchemadeclaresproperties: { tenantId: { type: 'string', 'x-mcp-header': 'X-Tenant-Id' } }— valid per the new JSDoc. - A spec-compliant client calling that tool mirrors the
tenantIdargument into anX-Tenant-Idheader on thetools/callPOST. The SDK client transport, by contrast, sends only its common/auth/session/protocol-version headers — the annotated argument is never mirrored, so the request from an SDK client is missing the header a spec-compliant server (or intermediary routing on it) expects. - Conversely, a spec-compliant server receiving a
tools/callfrom a spec-compliant client is expected to apply the transport spec's validity/extraction rules — including rejecting a header↔body mismatch withHeaderMismatchError(-32001, HTTP 400). The SDK server transport performs no such extraction or validation, so mismatched or spoofed header values pass through silently. - Nothing in CI flags either gap, because the transports have no compile-time link to the spec snapshot and the rule is prose, not a type shape.
Impact and how to fix
This is not a defect introduced by this automated sync — the PR is already held pending the companion 2026-07-28 redesign — but it is a transport-level feature the existing companion-work comments do not enumerate. The companion port should: (a) on the client transport, when sending tools/call, consult the tool's inputSchema (or an equivalent registration-time map) for x-mcp-header annotations and mirror the corresponding argument values into POST headers per the transport spec's validity rules; (b) on the server transport, extract those headers, cross-validate them against the body arguments, and reject mismatches/missing/malformed headers with the -32001 HeaderMismatchError envelope as HTTP 400 (which dovetails with the error-code→HTTP-status hook already called for in #3239359153); and (c) add the corresponding ProtocolErrorCode / schema plumbing tracked in the sibling -32001 finding.
4c6b66f to
20baa74
Compare
| * corresponding values in the request body, or required headers are | ||
| * missing or malformed. | ||
| * | ||
| * @category Errors | ||
| */ | ||
| export const HEADER_MISMATCH = -32020; | ||
|
|
||
| /** | ||
| * Error code returned when a server requires a client capability that was | ||
| * not declared in the request's `clientCapabilities`. | ||
| * | ||
| * @category Errors | ||
| */ | ||
| export const MISSING_REQUIRED_CLIENT_CAPABILITY = -32003; | ||
| export const MISSING_REQUIRED_CLIENT_CAPABILITY = -32021; | ||
|
|
||
| /** | ||
| * Error code returned when the request's protocol version is not supported | ||
| * by the server. | ||
| * | ||
| * @category Errors | ||
| */ | ||
| export const UNSUPPORTED_PROTOCOL_VERSION = -32004; | ||
| export const UNSUPPORTED_PROTOCOL_VERSION = -32022; |
There was a problem hiding this comment.
🔴 This re-pull repartitions the MCP error-code range (-32000..-32019 implementation-defined, -32020..-32099 spec-reserved) and renumbers the spec constants — HEADER_MISMATCH is now -32020, MISSING_REQUIRED_CLIENT_CAPABILITY moves -32003 → -32021, and UNSUPPORTED_PROTOCOL_VERSION moves -32004 → -32022 — so the SDK's ProtocolErrorCode.MissingRequiredClientCapability/UnsupportedProtocolVersion members (enums.ts:19/24) and the public UnsupportedProtocolVersionError class (errors.ts) now carry the wrong wire values for the 2026-07-28 revision they document, and the spec.types.2026-07-28.test.ts assertions pinning -32003/-32004 fail. The new partition also makes prior comment #3433435413 partially stale: the transport's -32000/-32001 codes (e.g. the 404/-32001 'Session not found' response) now sit in the implementation-defined sub-range and no longer collide with HeaderMismatchError, so any ProtocolErrorCode.HeaderMismatch member added by the companion work must use -32_020, not -32_001. The remaining HeaderMismatchError gap (no SDK schema/types/specTypeSchema registration, no sdkTypeChecks or allowlist entry, and the snapshot now exporting 151 types vs the hardcoded toHaveLength(150)) still stands and keeps the 2026-07-28 drift guard red.
Extended reasoning...
What changed
This re-pull introduces an explicit partition of the JSON-RPC implementation-defined error range (spec.types.2026-07-28.ts:383-401): -32000 to -32019 is implementation-defined ("the specification will never define codes in this sub-range"), and -32020 to -32099 is reserved for spec-defined codes, allocated sequentially from -32020. Under that scheme the constants are renumbered: HEADER_MISMATCH = -32020 (line 410), MISSING_REQUIRED_CLIENT_CAPABILITY moves from -32003 to -32021 (line 418), and UNSUPPORTED_PROTOCOL_VERSION moves from -32004 to -32022 (line 426). The old -32002/-32042 codes are documented as "reserved and never reused" legacy values from earlier revisions.
SDK surface that is now stale
The SDK recently added (per the 2026-06-17 status update) ProtocolErrorCode.MissingRequiredClientCapability = -32_003 and ProtocolErrorCode.UnsupportedProtocolVersion = -32_004 (packages/core/src/types/enums.ts:19,24), each JSDoc-documented as "protocol revision 2026-07-28". packages/core/src/types/errors.ts:59-69 builds the publicly exported UnsupportedProtocolVersionError class on the -32_004 enum value (and types.ts:488 documents "the -32004 ... protocol error"), and test/types/errors.test.ts asserts code -32004. After this sync those members carry the wrong wire values for the protocol revision they explicitly claim to implement: a 2026-07-28-compliant peer switching on error.code === -32021/-32022 will not recognise -32003/-32004, and the renumbered spec relegates those values to never-reused legacy status. This is not silent, either — packages/core/test/spec.types.2026-07-28.test.ts:504-507 imports the spec constants and asserts they equal -32003/-32004 and equal the corresponding ProtocolErrorCode members, so this sync directly breaks those assertions (-32003 !== -32021, -32004 !== -32022).
Prior -32001 guidance is now stale
Inline comment #3433435413 was written against the previous revision where HEADER_MISMATCH = -32001, and its main remediation items were (a) resolve the wire-code collision with the Streamable HTTP transport's createJsonErrorResponse(404, -32_001, 'Session not found') (packages/server/src/server/streamableHttp.ts:887) and (b) add ProtocolErrorCode.HeaderMismatch = -32_001. Under the new partition the transport's -32000/-32001 responses sit squarely in the implementation-defined sub-range, so the collision concern is moot — the session-not-found response and its pinned tests do not need re-coding for this reason — and a porter who follows the old guidance literally would bake the wrong wire value (-32_001 instead of -32_020) into the public ProtocolErrorCode enum.
What remains valid from the prior comment
The missing SDK surface for HeaderMismatchError is still real, just against the new value: grep HeaderMismatch across packages/core/src matches only the spec snapshot — there is no ProtocolErrorCode member for -32_020, no HeaderMismatchErrorSchema, no types.ts alias, and no specTypeSchema registration. The drift guard reflects this: spec.types.2026-07-28.test.ts has neither an sdkTypeChecks entry nor a MISSING_SDK_TYPES_2026_07_28 allowlist entry for HeaderMismatchError (only the MissingRequiredClientCapabilityError/UnsupportedProtocolVersionError envelopes are allowlisted at lines 477-478), so the coverage check at line 539 fails on this name, and the snapshot now exports 151 types versus the hardcoded toHaveLength(150) at line 521.
Step-by-step proof
- Diff:
MISSING_REQUIRED_CLIENT_CAPABILITYchanges from-32003to-32021andUNSUPPORTED_PROTOCOL_VERSIONfrom-32004to-32022;HEADER_MISMATCHis added at-32020with the partition comment declaring-32000..-32019implementation-defined. enums.ts:19,24still ship-32_003/-32_004documented as 2026-07-28;errors.tsexportsUnsupportedProtocolVersionErroremitting-32_004viaexports/public/index.ts:97.spec.types.2026-07-28.test.ts:504-507assertsProtocolErrorCode.MissingRequiredClientCapability === MISSING_REQUIRED_CLIENT_CAPABILITY(and likewise for UnsupportedProtocolVersion) → both fail after this sync (-32003vs-32021,-32004vs-32022).- Separately,
extractExportedTypespicks upHeaderMismatchError; it is in neithersdkTypeChecksnor the allowlist → coverage check fails, and 151 ≠ 150 fails the count check. - A 2026-07-28-compliant server emitting
-32021/-32022is not recognised by SDK code switching on the current enum values; conversely an SDK server emitting-32003/-32004sends codes the revision now treats as unassigned legacy values.
How to fix (companion work)
Renumber ProtocolErrorCode.MissingRequiredClientCapability to -32_021 and ProtocolErrorCode.UnsupportedProtocolVersion to -32_022 for the 2026-07-28 surface (or add new members if interop with the prior draft values is desired), update the UnsupportedProtocolVersionError/types.ts JSDoc and errors.test.ts, and fix the spec.types.2026-07-28.test.ts constant assertions. When adding the HeaderMismatch surface called for in #3433435413, use -32_020 (not -32_001), add the schema/alias/specTypeSchema registration or an allowlist entry, and bump the export-count assertion to 151. Drop the now-moot 'decide the -32001 session-not-found collision' items from the porting checklist — the transport's implementation-defined codes no longer clash.
| /** | ||
| * The ID of the request to cancel. | ||
| * | ||
| * This MUST correspond to the ID of a request previously issued in the same direction. | ||
| * This MUST correspond to the ID of a request the client previously issued. | ||
| */ | ||
| requestId?: RequestId; | ||
| requestId: RequestId; |
There was a problem hiding this comment.
🔴 This sync makes CancelledNotificationParams.requestId required ('MUST correspond to the ID of a request the client previously issued'), but the SDK's CancelledNotificationParamsSchema still declares requestId: RequestIdSchema.optional() (packages/core/src/types/schemas.ts:215) with the now-stale 'previously issued in the same direction' JSDoc — so the bidirectional drift-guard checks for CancelledNotificationParams and CancelledNotification in spec.types.2026-07-28.test.ts (lines 117/121) now fail TS2322 in the spec = sdk direction, and neither type is in the MISSING_SDK_TYPES_2026_07_28 allowlist. Note that prior comment #3368827092 concluded 'no SDK change is needed now' when both sides were optional — that conclusion is now stale; the companion work needs to either keep the field optional for 2025-11-25 interop and carve these two entries out of the 2026-07-28 guard, or split a 2026-07-28-strict schema, plus update the stale JSDoc at schemas.ts:213 and the Protocol._oncancel no-op path that exists only for an absent requestId.
Extended reasoning...
What changed in the spec
This re-pull (91b403f8) changes CancelledNotificationParams.requestId from optional (requestId?: RequestId) to required (requestId: RequestId) at spec.types.2026-07-28.ts:586-591, and rewrites the JSDoc from 'previously issued in the same direction' to 'the ID of a request the client previously issued.' The parent CancelledNotification JSDoc (line ~600) is also rewritten: the notification is now client→server for ordinary cancellation, with a narrowly scoped server→client use on stdio solely to terminate a subscriptions/listen stream.
What the SDK still ships
CancelledNotificationParamsSchema at packages/core/src/types/schemas.ts:215 still declares requestId: RequestIdSchema.optional(), and the JSDoc at schemas.ts:213 still reads 'previously issued in the same direction' — wording this diff deletes from the spec. Downstream, Protocol._oncancel (packages/core/src/shared/protocol.ts) contains a no-op branch that exists solely to tolerate an absent requestId (if (!notification.params.requestId) { return; }), a code path that only made sense under the 2025-11-25 task-era optionality.
How the drift guard breaks
packages/core/test/spec.types.2026-07-28.test.ts has explicit bidirectional sdkTypeChecks entries for CancelledNotificationParams (line 117) and CancelledNotification (line 121), each doing sdk = spec; spec = sdk;. Neither name appears in the MISSING_SDK_TYPES_2026_07_28 allowlist, so both checks are live.
Step-by-step proof:
- Post-sync,
SpecTypes.CancelledNotificationParams.requestIdisRequestId(required). - The SDK's Zod-inferred
SDKTypes.CancelledNotificationParams.requestIdisRequestId | undefined(from.optional()at schemas.ts:215). - The
spec = sdkassignment at test line 117 fails with TS2322:requestIdmay beundefinedin the SDK type but is required in the spec type. - The same failure repeats for the nested
paramsofCancelledNotificationat test line 121. - So the 2026-07-28 drift-guard test file no longer typechecks against this sync — a CI-visible failure introduced by this diff.
Why the existing porting checklist points away from this
Prior comment #3368827092 (2026-06-07) examined this exact field while both sides were optional and explicitly concluded: 'the bidirectional CancelledNotification assignability check in spec.types.test.ts passes in both directions … No SDK change is appropriate now.' The 2026-06-18 status update #3433356219 notes the field became required only as the resolution of the ServerNotification union-arm reachability discussion; it never names the schemas.ts:215 divergence or the now-failing drift-guard entries. A porter working through the existing comments would therefore treat this field as settled and miss a real TS2322 failure plus an unrecorded companion-work decision.
Impact
CI for the 2026-07-28 drift guard goes red on these two entries (on top of the other documented breakage on this held sync). More importantly, the companion work has a genuine wire-compatibility decision to make that no existing comment records: the 2025-11-25 snapshot (and the deliberately retained 2025-11-25 wire surface from #2248) still permits requestId-less cancellations from task-era peers, so the SDK receiver schema arguably should stay optional for that interop window — but then the 2026-07-28 guard needs a deliberate carve-out, not a silent failure.
How to fix (companion work)
Pick one of:
- Keep
requestIdoptional inCancelledNotificationParamsSchemafor 2025-11-25 interop, and moveCancelledNotificationParams/CancelledNotificationinto theMISSING_SDK_TYPES_2026_07_28allowlist (or add an explicit carve-out) so the 2026-07-28 guard documents the divergence instead of failing. - Split a 2026-07-28-strict schema (required
requestId) keyed to the new revision, keeping the lenient schema for the older snapshot.
Either way, also update the stale 'previously issued in the same direction' JSDoc at schemas.ts:213 to the new client-direction wording (plus the stdio subscriptions/listen-termination caveat), and revisit the Protocol._oncancel absent-requestId no-op path — under 2026-07-28 semantics an absent requestId is no longer a valid wire state, so that branch should either be scoped to the 2025-11-25 interop window or removed when the strict schema lands.
20baa74 to
234b0fc
Compare
| */ | ||
| message: string; | ||
|
|
||
| /** | ||
| * The ID of the elicitation, which must be unique within the context of the server. | ||
| * The client MUST treat this ID as an opaque value. | ||
| */ | ||
| elicitationId: string; | ||
|
|
||
| /** | ||
| * The URL that the user should navigate to. | ||
| * |
There was a problem hiding this comment.
🟡 This sync deletes the URL-elicitation completion flow from the 2026-07-28 revision (elicitationId removed from ElicitRequestURLParams, ElicitationCompleteNotification deleted and dropped from ServerNotification), but the SDK's public runtime surface for that flow survives unannotated and is named by no existing comment: Server.createElicitationCompletionNotifier() (server.ts:588-606, no @deprecated/revision-scoping JSDoc unlike the SEP-2577 siblings in the same file), the assertNotificationCapability case for notifications/elicitation/complete (server.ts:301-308), elicitInput() URL mode still forcing callers to mint an elicitationId that has no meaning in 2026-07-28, and examples/{server,client}/src/elicitationUrlExample.ts building their flow on it. Companion work should version-scope or deprecate the notifier API and capability-gate case, make elicitationId optional/revision-dependent in the URL-mode params, and migrate the examples — the schema/test surface is already covered by #3433435404; this is the runtime sibling, kept (not removed) for the 2025-11-25 interop window.
Extended reasoning...
What changed in the spec
This re-pull (e3f281c7) removes the URL-elicitation completion machinery from the 2026-07-28 revision: the required elicitationId field is deleted from ElicitRequestURLParams (the post-sync interface at spec.types.2026-07-28.ts:2721-2738 is just { mode, message, url }), ElicitationCompleteNotification (notifications/elicitation/complete) is deleted outright, and it is dropped from the ServerNotification union. Under 2026-07-28 there is therefore no protocol method for signalling out-of-band elicitation completion and no ID with which to correlate it.
What the SDK still ships at runtime (not covered by the existing schema/test comment)
Inline comment #3433435404 already covers the schema/test surface for this deletion (ElicitRequestURLParamsSchema at schemas.ts:1994, the ElicitationComplete schemas at schemas.ts:2021-2036, types.ts aliases, specTypeSchema.ts registration, ServerNotificationSchema membership, and the spec.types.2026-07-28.test.ts TS2741/TS2694 failures). What no existing comment names is the live, non-deprecated public server runtime API that emits this flow on the wire:
Server.createElicitationCompletionNotifier(elicitationId, options)—packages/server/src/server/server.ts:588-606. A public method with no@deprecatedand no protocol-revision scoping in its JSDoc — in contrast to the SEP-2577-deprecated siblings in the same file (createMessage()/listRoots()etc., which carry "@deprecated Deprecated as of protocol version 2026-07-28"). It gates on_clientCapabilities.elicitation.urland emits{ method: 'notifications/elicitation/complete', params: { elicitationId } }— a notification method the 2026-07-28 revision no longer defines, and for which the new revision provides no delivery channel (theSubscriptionFilteropt-in allowlist has no field for it, and the standalone GET stream is replaced bysubscriptions/listen).- The
assertNotificationCapability()case'notifications/elicitation/complete'— server.ts:301-308 — the only capability gate for that method; it keys the deleted notification off_clientCapabilities.elicitation.url, a capability that says nothing about completion notifications in the new revision. server.elicitInput()URL mode — server.ts:528-538 sends the caller-suppliedElicitRequestURLParamson the wire, and because the SDK type/schema still requireselicitationId, every server author is forced to mint and transmit an ID that has no meaning in 2026-07-28 and whose completion-signal channel no longer exists. (The schema-side relaxation is in #3433435404; the API-shape consequence forelicitInputcallers is not.)- Examples:
examples/server/src/elicitationUrlExample.tsbuilds its whole flow oncreateElicitationCompletionNotifier+ elicitationId tracking (lines 55-107, 159-205), andexamples/client/src/elicitationUrlExample.ts:571-581registers anotifications/elicitation/completehandler keyed byelicitationId. (#3440492937 mentions these example apps only in the -32042/UrlElicitationRequiredErrorcontext — not the completion-notifier API.)
Why this is silent
server.ts and the example apps have no compile-time link to spec.types.2026-07-28.ts; the only CI signal from this deletion is the test-file breakage already enumerated in #3433435404. A porter who fixes the schemas/tests per that comment ships an SDK that still publicly exposes, capability-gates, and demonstrates an emission path for a notification method the new revision deleted — the same silent-runtime-surface class as the accepted -32042 (#3216586348), session-bootstrap (#3246176899), and logLevel-default (#3256562384) findings, which this PR's review trail consistently treats as distinct from their schema-side siblings.
Step-by-step scenario
- The companion 2026-07-28 work lands following the existing checklist:
ElicitRequestURLParamsSchemadropselicitationId, the ElicitationComplete schemas are scoped to the 2025-11-25 snapshot, and the drift-guard test is fixed. - A server author following the still-published, still-undeprecated API calls
server.createElicitationCompletionNotifier(id)after a URL elicitation against a 2026-07-28 peer. - The capability gate passes (the client declared
elicitation.url), and the server emitsnotifications/elicitation/complete— a method that does not exist in the negotiated revision. A strict 2026-07-28 client has no schema for it and, per the new opt-in subscription model, should never receive an unsolicited notification it didn't subscribe to. The intended replacement signal (re-poll / retry-with-inputResponsesunder theInputRequiredResultflow) is never produced.
How to fix (companion-work scope)
The SDK's wire surface deliberately targets 2025-11-25 (where this flow still exists), so removal is not the ask — version-scoping/deprecation is, mirroring how the SEP-2577 tier and the -32042 mechanism (#3440492937) were handled:
- Add revision-scoping or
@deprecatedJSDoc toServer.createElicitationCompletionNotifier()noting it is a 2025-11-25-only mechanism, and decide its behaviour when the negotiated revision is 2026-07-28 (no-op vs. throw). - Mirror that decision in the
assertNotificationCapabilitycase at server.ts:301-308. - Make
elicitationIdoptional / revision-dependent in theelicitInput()URL-mode parameter type so 2026-07-28 servers are not forced to mint a dead field (dovetails with the schemas change in #3433435404). - Migrate or fork
examples/{server,client}/src/elicitationUrlExample.ts(and any docs referencing the completion notification) to the 2026-07-28 completion model when that work lands.
Filed as a nit in the same companion-work-scoping tier as the other accepted silent-divergence findings on this held PR; it does not independently break CI.
| } | ||
|
|
||
| /** | ||
| * This notification can be sent by either side to indicate that it is cancelling a previously-issued request. | ||
| * This notification is sent by the client to indicate that it is cancelling a request it previously issued. | ||
| * | ||
| * On stdio, the server also sends this notification, solely to terminate a {@link SubscriptionsListenRequest | subscriptions/listen} stream: it references the ID of the `subscriptions/listen` request that opened the stream. Servers MUST NOT use this notification to cancel any other request. | ||
| * |
There was a problem hiding this comment.
🟡 Companion-work scoping note: the rewritten CancelledNotification JSDoc (lines 597–603) adds a normative server→client use on stdio — the server terminates a subscriptions/listen stream by sending notifications/cancelled referencing the ID of the listen request the client issued — but Protocol._oncancel (packages/core/src/shared/protocol.ts:345-352) only looks up _requestHandlerAbortControllers (incoming-request IDs) and never consults _responseHandlers/_progressHandlers, so a cancellation naming the receiver's own outgoing listen request is silently dropped and the pending request would hang until a misleading RequestTimeout. When the companion subscriptions/listen work lands, the cancelled-notification path needs a receiver-side route that settles/aborts the local outstanding listen request (gated per the spec's 'MUST NOT cancel any other request' constraint).
Extended reasoning...
What changed in the spec
This re-pull rewrites the CancelledNotification JSDoc (spec.types.2026-07-28.ts:597-603). The notification is now client→server for ordinary cancellation, and it gains one new normative server→client use: "On stdio, the server also sends this notification, solely to terminate a subscriptions/listen stream: it references the ID of the subscriptions/listen request that opened the stream. Servers MUST NOT use this notification to cancel any other request." In other words, the receiver of the cancellation is now expected to react to a cancellation that names a request the receiver itself issued (its own outgoing listen request) — not a request it is currently handling.
What the SDK does today
Protocol._oncancel (packages/core/src/shared/protocol.ts:345-352) handles every incoming notifications/cancelled by looking up this._requestHandlerAbortControllers.get(params.requestId) — a map keyed exclusively by incoming request IDs the local side is handling (populated in _onrequest at protocol.ts:512). It never consults _responseHandlers / _progressHandlers / _timeoutInfo (the maps tracking the local side's own outgoing requests, protocol.ts:289-291). A cancelled notification whose requestId names one of the receiver's own outstanding outgoing requests therefore finds no abort controller and silently returns: the pending Protocol.request() promise is never settled, no handler is informed, and nothing is cleaned up.
Step-by-step proof (once the companion subscriptions/listen work lands)
- An SDK client on stdio opens the notification stream by issuing
subscriptions/listen(the 2026-07-28 replacement for the standalone GET stream). The request ID lands in_responseHandlers/_progressHandlers. - The server later shuts the stream down using the only spec-defined mechanism on stdio: it sends
notifications/cancelledwith the listen request's ID. - The SDK client's default
notifications/cancelledhandler (registered in theProtocolconstructor, protocol.ts:323-325) routes to_oncancel. _oncancellooks up_requestHandlerAbortControllers.get(requestId)— the listen request is an outgoing request, so there is no entry — and returns.- The client never learns the stream was terminated: the listen request stays pending in
_responseHandlersuntil its timeout fires with a misleadingRequestTimeout, the subscription bookkeeping is never torn down, and there is no hook asubscriptions/listenimplementation could use to observe the termination.
Why nothing in CI surfaces it and why existing comments don't cover it
shared/protocol.ts does not import the spec snapshot, so the drift guard cannot see this — the same silent companion-work class as the other accepted transport/Protocol-layer findings on this PR. Status update #3433356219 records only the sender-side scoping of this change ('the server-side cancel path is now narrowly scoped to stream termination on stdio') for the ServerNotificationSchema work; #3440566896 covers the requestId optional-vs-required schema/test divergence and the absent-requestId no-op branch in _oncancel; the broader subscriptions/listen comments (#3239359153, #3223937258, #3433435417) cover transport routing, schemas, and subscriptionId stamping. None names the fact that _oncancel has no path for cancellations targeting the receiver's own outgoing requests — which is the mechanism the new normative sentence depends on.
Impact
Until addressed, the only spec-defined way to end a subscriptions/listen stream on stdio is a no-op against the SDK: the listen request hangs, then fails with a timeout error that misattributes the cause, and the listen-stream manager has no termination signal to clean up against.
How to fix (companion work, not this automated sync)
When implementing subscriptions/listen, extend the cancelled-notification handling so that a cancellation referencing one of the local side's outstanding outgoing requests settles/aborts that request (e.g. resolve or reject the _responseHandlers entry and clean up _progressHandlers/_timeoutInfo), or route it to the listen-stream manager — and gate the behavior to the listen request specifically, per the spec's 'Servers MUST NOT use this notification to cancel any other request' constraint.
| * toward `-32099`. | ||
| * | ||
| * Codes defined by earlier protocol versions remain reserved and are never | ||
| * reused: `-32002` (resource not found, 2025-11-25 and earlier; replaced by | ||
| * `-32602`) and `-32042` (URL elicitation required, 2025-11-25 only). |
There was a problem hiding this comment.
🟡 Companion-work item: this sync's new error-code partition prose (spec.types.2026-07-28.ts:398-400) retires -32002 — "resource not found, 2025-11-25 and earlier; replaced by -32602" — but the SDK still defines ProtocolErrorCode.ResourceNotFound = -32_002 (packages/core/src/types/enums.ts:14, with no version-scoping JSDoc, unlike the adjacent 2026-07-28-annotated members) and still throws it for unknown resource URIs in the resources/read handler (packages/server/src/server/mcp.ts:427), so a 2026-07-28-compliant client expecting -32602 won't recognize the response. The companion 2026-07-28 work should map resource-not-found to -32602 (InvalidParams) when speaking the new revision — keeping -32002 for the 2025-11-25 interop window — and/or add version-scoping JSDoc to the enum member, mirroring the -32042/UrlElicitationRequired treatment.
Extended reasoning...
What changed in the spec
This sync adds the new error-code partition comment (spec.types.2026-07-28.ts:396-400), which states that codes defined by earlier protocol versions remain reserved and are never reused, explicitly listing -32002 (resource not found, 2025-11-25 and earlier; replaced by -32602). In other words, under the 2026-07-28 revision the dedicated resource-not-found code is gone: an unknown-resource error folds into the standard INVALID_PARAMS (-32602), and -32002 becomes a never-reused legacy value with no meaning for a 2026-07-28 peer.
What the SDK still ships
packages/core/src/types/enums.ts:14—ProtocolErrorCode.ResourceNotFound = -32_002, with no version-scoping JSDoc. The asymmetry is visible in the same enum: the immediately following membersMissingRequiredClientCapability = -32_003andUnsupportedProtocolVersion = -32_004both carry explicit "protocol revision 2026-07-28" notes, whileResourceNotFoundgives no hint that it is a 2025-11-25-and-earlier-only code.packages/server/src/server/mcp.ts:427— theresources/readhandler throwsnew ProtocolError(ProtocolErrorCode.ResourceNotFound, ...)when no registered resource or template matches the requested URI, so the SDK actively emits-32002on the wire for every unknown-resource read.
Why nothing in CI surfaces it
Neither enums.ts nor mcp.ts imports the spec snapshot, and the partition prose is JSDoc commentary rather than an exported type, so neither tsc nor the spec.types.2026-07-28.test.ts drift guard trips on this. It is the same silent-divergence class as the accepted findings on this PR for -32042/UrlElicitationRequired, the session bootstrap, and the -32021/-32022 error-code renumbering — and unlike those, none of the ~50 existing comments names -32002/ResourceNotFound or the mcp.ts emission site (the existing error-code coverage is limited to -32001/-32003/-32004/-32020/-32021/-32022/-32042; -32002 appears only in passing as a reserved legacy value). A porter working through the existing checklist would therefore miss this item.
Step-by-step proof
- The companion 2026-07-28 implementation lands and the SDK negotiates the new revision with a spec-compliant client.
- The client calls
resources/readwith a URI the server does not serve. - The handler at
mcp.ts:427throwsProtocolError(ProtocolErrorCode.ResourceNotFound), whichProtocolserialises as a JSON-RPC error withcode: -32002. - Per the 2026-07-28 partition,
-32002is an unassigned legacy value — the spec says the unknown-resource case is now signalled with-32602(INVALID_PARAMS). A client switching onerror.code === -32602for "resource not found" does not recognize the response and falls into its generic-error path instead of its resource-not-found handling. - Conversely, an SDK client built only against
ProtocolErrorCode.ResourceNotFoundwill not classify a compliant 2026-07-28 server's-32602unknown-resource response as resource-not-found.
Why this is not an immediate defect
The SDK currently targets 2025-11-25 (LATEST_PROTOCOL_VERSION = '2025-11-25' in constants.ts), where -32002 is still the correct wire code, and this PR is an automated sync already held pending the companion 2026-07-28 redesign. So nothing breaks today — this is forward-looking companion-work scoping, in the same tier as the other accepted error-code items on this PR.
How to fix (companion-work scope)
- When the negotiated revision is 2026-07-28, map unknown-resource errors in the
resources/readpath (mcp.ts:427) toProtocolErrorCode.InvalidParams(-32602), keeping-32002for the 2025-11-25 interop window (version-aware emission, mirroring how the other renumbered codes are being handled). - Add version-scoping JSDoc to
ProtocolErrorCode.ResourceNotFound("2025-11-25 and earlier; replaced by-32602in 2026-07-28"), matching the treatment already applied toUrlElicitationRequired/-32042and the 2026-07-28-annotated members beside it. - Optionally note the mapping in the migration/companion checklist so client-side code that special-cases
ResourceNotFoundknows to also accept-32602from 2026-07-28 servers.
234b0fc to
2b373fd
Compare
2b373fd to
2b8cddf
Compare
This PR updates
packages/core/src/types/spec.types.2026-07-28.tsfrom the Model Context Protocol specification.Source file: https://github.com/modelcontextprotocol/modelcontextprotocol/blob/e3f281c7243b7dfe4c0dca450230fe3d7de3cf8b/schema/draft/schema.ts
This is an automated update triggered by the nightly cron job.